Adds --with-dependencies flag #650
Conversation
|
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-05-29 19:35:11 UTC |
JayjeetAtGithub
force-pushed
the
jayjeet/issue625
branch
from
584f6a9
to
3231a72
Compare
|
@ivotron Please review. It's ready. Thanks ! |
There was a problem hiding this comment.
thanks a lot @JayjeetAtGithub! please take a look at the comments
ci/test/actions-demo
Outdated
|
|
||
| # test --with-dependencies flag | ||
| popper run test | ||
| popper run --with-dependencies test |
There was a problem hiding this comment.
can we check the output in order to ensure that only dependent actions get executed? Also, we can skip the previous popper run test as that functionality is being checked somewhere else
There was a problem hiding this comment.
@ivotron Are we required to match the output with a hardcoded output ? If thats the case, we might use --dry-run flag.
There was a problem hiding this comment.
yeah, using --dry-run would speedup the execution. No need to compare to hardcoded strings though. We could run with and without --with-dependencies and ensure that the latter is including dependent actions in the execution
There was a problem hiding this comment.
@ivotron On running without --with-dependencies, the actions defined in the needs attribute of the referenced action will get executed ?
There was a problem hiding this comment.
no, only the given action
cli/popper/gha.py
Outdated
| @@ -157,7 +157,7 @@ def instantiate_runners(self): | |||
| a, self.workspace, self.env, | |||
| self.dry_run) | |||
|
|
|||
| def run(self, action_name=None, reuse=False, parallel=False): | |||
| def run(self, action_name=None, with_dependencies=False): | |||
There was a problem hiding this comment.
we can simplify the implementation of this feature by extending the Workfow.get_stages() function so that it can be given an action where to start generating stages. In this way, the logic of running a workflow stays the same (i.e. no modifications would be needed).
|
@ivotron Please have a look at the approach. |
cli/popper/gha.py
Outdated
| if action_name: | ||
| self.wf.get_runner(action_name).run(reuse) | ||
| if self.action_name: | ||
| if self.with_dependencies: |
There was a problem hiding this comment.
looks like this condition is doing the same as the outer else below (lines 195-196), right? Can we instead remove it?
cli/popper/parser.py
Outdated
| @@ -79,25 +79,35 @@ def get_stages(self): | |||
| 'next', set())) | |||
| current_stage = next_stage | |||
|
|
|||
| def complete_graph(self): | |||
| def complete_graph(self, action_name=None): | |||
There was a problem hiding this comment.
can we update the docstring so it explains the purpose of this new parameter? Is this new parameter restricting how deep the completion goes? I feel like the code is a bit difficult to parse.
Conceptually, it might be more readable to let the complete_graph work on the complete graph and then implement any sort of filtering or ad-hoc graph traversal separately. So in this case, we could leave the complete_graph function as it was before, and instead add an optional action parameter to the get_stages method that indicates that we want to traverse the graph from the root to the given action (as opposed to going from the root to the end of the graph when this action parameter is None), ignoring any nodes that are not a direct dependency to this given action
JayjeetAtGithub
force-pushed
the
jayjeet/issue625
branch
from
9e86b99
to
27e93c2
Compare
|
@ivotron Please have a look ! I have followed the approach as of filtering actions but now with dependencies. |
|
thanks a lot! |
Adds ability to run an action and all of its dependencies (up to that action) fixes #625
Adds ability to run an action and all of its dependencies (up to that action) fixes #625
Adds ability to run an action and all of its dependencies (up to that action) fixes #625
Adds ability to run an action and all of its dependencies (up to that action) fixes #625
Addresses #625