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

Allow flags for commit message driven execution #703

Merged
merged 14 commits into from
Jul 10, 2019

Conversation

JayjeetAtGithub
Copy link
Collaborator

@JayjeetAtGithub JayjeetAtGithub commented Jul 4, 2019

Addresses #696

@pep8speaks
Copy link

pep8speaks commented Jul 4, 2019

Hello @JayjeetAtGithub! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-10 05:37:38 UTC

@JayjeetAtGithub
Copy link
Collaborator Author

JayjeetAtGithub commented Jul 5, 2019

@ivotron I was using action argument to take both action and workflows, but on moving ahead with that I found that it increased the complexity of the code very much, because of a large number of type checkings, if else, etc. Same was the case with --skip taking workflows.

So I was thinking if we could keep the --wfile flag intact and just make it multiple=True. Also we can change the action argument to a flag --action instead. It would be very helpful, if you could review the current PR and also please give your thoughts upon this. Thanks.

@ivotron
Copy link
Collaborator

ivotron commented Jul 8, 2019

I see. The main problem I see with allowing multiple occurrences of --wfile is that all other arguments (except --recursive) assume there's only one workflow being executed. For example --skip or --on-failure, etc..

One alternative we have is to go with the original plan of supporting multiple occurrences of the popper:run[...] keyword, and run as many times as this is shown. If we do this, we could deprecate --wfile and parse the positional argument with the following criteria: if there's a file with that name, then assume it's a workflow, otherwise assume it's an action.

@JayjeetAtGithub JayjeetAtGithub force-pushed the jayjeet/cmd_run branch 3 times, most recently from 296eb5f to 1d07e76 Compare July 8, 2019 19:58
@JayjeetAtGithub JayjeetAtGithub marked this pull request as ready for review July 8, 2019 22:20
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
cli/popper/commands/cmd_run.py Outdated Show resolved Hide resolved
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.

None yet

3 participants