fix: bring back --up flag #255
Conversation
|
LGTM. The integration test is a good addition. |
|
Hello @imheresamir @apruszynski @szh who could I ask to have a look at this PR when they have time? Thanks! |
|
@marco-m I'll bring this up to my team. Thanks! |
There was a problem hiding this comment.
Brilliant work @marco-m! Thank you for finding the root cause and addressing the issue comprehensively. I was responsible for the commit that introduced the issue so I doubly appreciate the addition of the high level test case to validate the feature!
| @@ -45,6 +45,14 @@ func RunSubprocess(sc *SubprocessConfig) (int, error) { | |||
|
|
|||
| subs := convertSubsToMap(sc.Subs) | |||
|
|
|||
| if sc.RecurseUp { | |||
| currentDir, err := os.Getwd() | |||
There was a problem hiding this comment.
Sigh I am missing checking the err returned by Getwd :-(
There was a problem hiding this comment.
I just force pushed a fix. Ready for review.
|
I'm going to try to get this merged on Friday. |
|
Can you please send a signed copy of the CLA to [email protected]? |
This is the original test when I introduced the --up flag Signed-off-by: Marco Molteni <[email protected]>
This simplifies the code Signed-off-by: Marco Molteni <[email protected]>
This time we add an integration test, to make it sure that a test will catch it. Signed-off-by: Marco Molteni <[email protected]>
Signed-off-by: Marco Molteni <[email protected]>
|
@szh I could not find a mention of having to sign a CLA in the CONTRIBUTING.md for this repo (summon). Reading https://github.com/cyberark/community/blob/main/CONTRIBUTING.md#when-the-repo-does-not-include-the-cla, I saw that in this case you are OK with a |
|
Perfect. This is all ready to merge internally, just waiting for one more layer of code review. |
|
Merged in the latest version! Thank you so much for your contribution! |
Desired Outcome
Fix #254 (flag --up is not working any more).
Implemented Changes
Describe how the desired outcome above has been achieved with this PR. In
particular, consider:
Added back the handling of --up flag, which has been removed by mistake (I think).
Best reviewed commit-per-commit, considering the commit messages.
No manual tests are required; the PR adds an integration test to protect from this regression. See the comments I added in #254 for details.
Connected Issue/Story
Resolves #254
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security