Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test_config.py on Windows #1247

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Fix test_config.py on Windows #1247

merged 3 commits into from
Feb 22, 2021

Conversation

ERYoung11
Copy link
Contributor

Description

I'm developing on Windows with VS Code. test_config.py failed for the final case as setting a directory to read-only did not prevent writing to that directory on Windows. I have updated the code to try name the subdir with an illegal character which prevents the creation of rcfile. I used the double-quote character. This character is legal on Linux. This test now passes on Linux (when run as non-root) and Windows.

Wrappager was also marked as changed due to, I'm guessing, some hidden characters. I have checked it in with this too. As you can see, github thinks there are no changes to the file.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

@ERYoung11 ERYoung11 changed the title Fix test config windows Fix test_config.py on Windows Feb 22, 2021
@ERYoung11
Copy link
Contributor Author

I'm no expert here, but I don't think this should have failed due to timeout as this was a very insignificant change.

@j-bennet
Copy link
Contributor

@ERYoung11 Yes, I don't think the test failure was related to your change. Thank you for the PR! Feel free to update changelog / authors file, or I can merge it as-is if you prefer.

@ERYoung11
Copy link
Contributor Author

Go ahead and merge. If I do something more significant I'll update those sections. I'm currently looking at Issue #1240.

Also, is the vagrant file out of date? I can't find the chef/centos & chef/debian it's asking for. I could fix that up too.

Is there somewhere that I can chat about these sorts of questions? Fair warning, I'm sort of a newbie (and old newbie working to learn new tricks).

@j-bennet j-bennet merged commit 9ca545d into dbcli:master Feb 22, 2021
@j-bennet
Copy link
Contributor

If I do something more significant I'll update those sections. I'm currently looking at Issue #1240.

Sounds good to me. Any and all contributions are appreciated.

Also, is the vagrant file out of date? I can't find the chef/centos & chef/debian it's asking for. I could fix that up too.

Yes, it is quite outdated. If you're willing to take on updating it, please do.

Is there somewhere that I can chat about these sorts of questions? Fair warning, I'm sort of a newbie (and old newbie working to learn new tricks).

We prefer to communicate via github issues. This gives us more visibility on when a contributor drops the ball / loses interest in the issue (happens all the time unfortunately), and someone else can pick it up, because all the context is there.

@ERYoung11 ERYoung11 deleted the fix_test_config_windows branch February 24, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants