-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[DOC] fix erronously named option "user" to "owner" in the docstring of "files.find" in salt/modules/files.py #67912
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
base: master
Are you sure you want to change the base?
Conversation
associated with def find(path, *args, **kwargs) in file.py
you need to fix pre-commit. |
Happy too. But, I don't know what the problem is. Do I execute pre-commit install again? Do I reinstall pre-commit with apt? |
nothing that drastic. this is the issue.
most likely you either had too many lines. or didn't have a line ending on the end of the file. |
I failed to install pre-commit to the branch containing the fixes for 67911. I did that, added an end of line to the file salt/modules/file.py, did a git add for file.py then committed. Pre-install then ran during the commit, but it output the following:
I don't know if the output "Fixing salt/modules/file.py" means pre-commit fixed the problem or if "fix end of files.........................................................Failed" means there is still a problem. I need some guidance on this before I do the push to my repo on GitHub. |
OK, I looked at file.py after pre-commit ran and it deleted the newline after the last non-empty line in the file. So, I must have misinterpreted, "most likely you either had too many lines. or didn't have a line ending on the end of the file." Can you explain or point me to the requirements for end-of-file on salt files? |
I think we require a single new line at the end of python files. |
Thanks. That is what I thought. However, I added a single blank line at the end of file.py (which didn't have one) and it seems pre-commit removed it when I committed the change. Also after pre-commit runs, the commit doesn't and file.py is in a strange state in which it is listed as "Changes to be committed:" as well as "Changes not staged for commit:". Here is the record showing this:
|
pre-commit can be finicky to work with. what i normally do is instead of committing right away. i run pre-commit will make some changes to files. but it doesn't add them to the commit on its own. so you have to account for it when working with it and git. |
I tried your suggestion. pre-commit ran and deleted the newline at the end of file.py. Then I ran pre-commit again and it passed. However, git status showed no files modified, so I couldn't add file.py and do the commit. Is it possible that pre-commit doesn't want a newline at the end of the file? If so, the failure of pre-commit when running the PR is hard to understand. Might the version of pre-commit which runs for the PR not be the same as the version of pre-commit installed on linux using apt? The version installed on my linux machine is 4.0.1 |
precommit in the PR isn't failing on file.py. it is failing on your changelog. also once you get a pre-commit to run clean and git add works. and you get a commit in place. don't forget to push to github. the PR won't update till you do the push. |
I looked at the changelog entry for 67911 ( 67911.fixed.md on https://github.com/dnessett/salt/ - changelog directory for branch bug fix-67911 - from which I did the pull request) and it does not have a newline after the changelog description. I then looked at the file on my local machine and it does have a newline after the description. So, I am speculating that running pre-commit on the PR side deleted the newline from the changelog file on my local repository on GitHub. I cannot commit the local file and push it, since git detects no changes to its content. pre-commit on my local machine does not complain that there is a newline after the description on 67911.fixed.md, which is puzzling. I am now completely confused. |
you have most likely rerun pre-commit so many times at this point that it is fixed in your local system. don't think about what it is trying to do and just let it do it. |
Thinking about this, to get my local system in sync with the repo branch, should I delete the newline on my local system changelog entry, git add that entry, run pre-commit until it is satisfied then commit the change and finally push the result? This will not change the file on my local repository, but will get my local machine and repo in sync. |
what does git status give? without touching anything. |
|
weird ... your git isn't tracking upstream? output of these?
|
It seems I forgot to set upstream for the branch. Actually, I did but set it to saltstack/salt
|
looks like bugfix-67911 is not set to track origin/bugfix-67911
also next time you create a new branch locally use -u when you push it for the first time to origin. that will set git to track the github version and see differences between origin and local |
OK. Thanks, FYI. I have to step a way for several hours. |
I executed I think the reason the git fetch did not sync the two copies is when the PR pre-commit ran, it changed the copy of 67911.fixed.md on my repo without using git commit logic. This caused the two copies to go out of sync. The |
so, about your speculation. about what fetch does. notice that it only lists remote-tracking branches as being updated. other wise it just downloads the meta data for branchs/tags. as noted your so what happened. your local copy was not tracking origin. you made updates. then didn't push those updates to origin. so they came out of sync. then started into a cycle of git commit, git fetch. trying to understand what was happening. but since git fetch doesn't do anything for local branches that are not tracking the git fetch did absolutely nothing. so just remember. git fetch is for when you want to pull from remote before doing a rebase or merge. |
Thanks for the explanation. |
What does this PR do?
Fixes a documentation error in salt/modules/files.py. Specifically, it corrects the erroneously named option "user" to "owner".
What issues does this PR fix or reference?
[Doc] 67911
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No