Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
NEW Source Control module: git add/commit/push #57
NEW Source Control module: git add/commit/push #57
Changes from 17 commits
8093f4f
37f8c8b
8502989
38f5da1
4b5768b
1505b4d
b56ea8c
17f6628
08c807a
a7b17c4
ef7ee5e
09f5e1d
d102c7c
ba76434
ce7a41f
d293967
4e09fb9
148d71f
3136f14
6e9cf85
741a52e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never ever use strings for commands! Use lists. This will also take care of all option quoting problems. (Your current version will break if someone uses
"
in a comment.)(Note that
module.run_command(cmd, ...)
accepts lists forcmd
.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure I understand properly:
Those strings are appended to a list that is then passed to
run_command
Is not that the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Consider
module.run_command('echo "Hello world!"')
vs.module.run_command(['echo', 'Hello world'])
. Here it is clear what are parameters 1 and 2 of the command, without having to parse it first with shell lexer rules.Also assume that the commit message is
x " hello " x
. Then your call would result in the arguments-m
,"x "
,hello
," x"
(four separate ones) passed togit
. Using['-m', 'x " hello " x']
would result in precisely two arguments being passed (with message exactly as provided by user).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean know. It makes absolutely sense. I will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding this bit, I have a problem to make it work.
This is the command:
"cmds": ["git", "add", "1585649737984992100.txt 1585649738063034300.txt"]
When I run it against testhost I get
{"changed": false, "cmd": "add", "msg": "[Errno 2] No such file or directory: b'add': b'add'", "rc": 2}
If I passe the command as string (using
' '.join()
) the command is successful. I had a look around to other modules for git_config and I see they use a string forrun_commands
. I am bit confused :/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because other modules do bad things doesn't mean you should copy their behavior :) Using lists is a lot better than strings for
run_command
.About that error: where exactly does that come from? Module output is not really helpful to understand what went wrong. What exactly did you pass to
run_command
, and what didrun_command
return?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, you should really get rid of this loop. It makes the module much harder to follow.
Just build and execute the commands sequentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if output is localized?
For
git commit
, you should use the--porcelain
option. No idea if something similar exists forgit add
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use
--porcelain
I am going to loose all the string outputs that I use for theoutput
anderror
checking. rc in git are pretty useless as it pretty much always return1
so I believe having strings to use as anchor for if statement is a good thing. If you have an idea how to implement a safeoutput
error
check using--porcelain
please let me know. Here an example of what I mean:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing the output/error checks on string output is a terrible idea. This doesn't work at all.
Also please read
man git-commit
on what--porcelain
does. In this case, empty output means "nothing to commit".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. What about for
git add
that does not support--porcelain
. Is ok to use string output or do you have other ideas? I was thinking to userc
but I am not sure if distinguish betweennothing to commit, working tree clean
andfatal: pathspec 'file' did not match any files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should run
git status --porcelain=v1
, like here: https://github.com/buildbot/buildbot/pull/5023/files#diff-380a2f2e80216ef42112b8b625db2516R753-R761Then you know whether there's something to commit or not.
Parsing the normal text output of git is something you should NEVER rely on. There exist translated versions of CLI programs, and your code will do the wrong things when encountering such translated output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failing when there's nothing to commit isn't useful in all cases. How about making that configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I would you call the argument?
allow_empty
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wait, I think it is better to distinguish between three behaviors:
--allow-empty
option).Maybe call the option
empty_commit
and make it an enum with valuesfail
,skip
andempty_commit
. Or something similar.(Now it also reminds me where I've seen this before: https://docs.buildbot.net/current/manual/configuration/buildsteps.html#gitcommit has an option
emptyCommits
. I remembered that I have actually added it because I needed it once :D Here's the PR: buildbot/buildbot#5023 that might help you with implementing this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok. Good suggestion. That line you indicated though checks if there are not files to add. The commit check is the line below:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developing the
empty_commit=skip
I realize that is not really useful in my opinion. Basically, if I am not mistakenskip
mode will allow the module just to stage file and personally I cannot see a use case where you want use a specific ansible module just to add files.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty_commit=skip
means: if there's nothing to add (stage), don't commit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
empty_commit=skip
would be conditional forgit add
command whileempty_commit=allow|fail
would be conditional forgit commit
. Is that correct? If that is the case, would not be confusing that one argument affect the behavior of 2 different thighs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all determines on what
git commit
does. Either it is potentially not run (skip
), it gets the--empty-commit
flag (allow
), or it is run as usual (fail
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here: use lists for commands, not strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And add
--porcelain
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break if
url
does not starts withhttps://
, since thenremote_add
has not been defined.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be just mater of indent that block under
if url.startswith('https://'):
Would that work for you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should rather raise an error when
url
does not start withhttps://
.