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! |
doodlesbykumbi
left a comment
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!
| 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 oss@cyberark.com? |
This is the original test when I introduced the --up flag Signed-off-by: Marco Molteni <marco.molteni@mailbox.org>
This simplifies the code Signed-off-by: Marco Molteni <marco.molteni@mailbox.org>
This time we add an integration test, to make it sure that a test will catch it. Signed-off-by: Marco Molteni <marco.molteni@mailbox.org>
Signed-off-by: Marco Molteni <marco.molteni@mailbox.org>
|
@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