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.

Sign up for GitHub

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

Jump to bottom

Option to traverse up the directory hierarchy looking for secrets.yml #144

Merged
merged 10 commits into from
Mar 25, 2020
Merged

Option to traverse up the directory hierarchy looking for secrets.yml #144

merged 10 commits into from
Mar 25, 2020

Conversation

marco-m
Copy link
Contributor

@marco-m marco-m commented Mar 20, 2020

close #122

@BradleyBoutcher @marco-m
Add upwards recursion when searching for a secretless config
f8f31f4 
- Unit tests added
- Accepts a path of a filename

Todo:
Stop searching when we reach the highest level dir,
currently, it continues to iterate
@marco-m
Copy link
Contributor Author

marco-m commented Mar 20, 2020 •
edited
Loading

@BradleyBoutcher I started from your branch, rebased and finished the work.

@izgeri
Copy link
Contributor

izgeri commented Mar 20, 2020

This is great, @marco-m! FYI @BradleyBoutcher is on PTO today, so he might not look at this until Monday.

I noticed that your PRs don't have a signoff message on them - can you either follow the instructions here to send us a signed CLA, or amend your commits to include the signoff? Thank you!!

https://github.com/cyberark/community/blob/master/Conjur/CONTRIBUTING.md#when-the-repo-does-not-include-the-cla

@marco-m
Attach the -u flag to the CLI parser
3b8e08a 
Signed-off-by: Marco Molteni <[email protected]>
@marco-m
git: ignore vscode files
50afb0e 
Signed-off-by: Marco Molteni <[email protected]>
@marco-m
walkFn: simplify code and tests by reducing mutation
85b93ba 
Signed-off-by: Marco Molteni <[email protected]>
@marco-m
walkFn: stop searching when reaching the root of the file system
2ad6c1d 
Signed-off-by: Marco Molteni <[email protected]>
@marco-m
walkFn: do not accept a file that is an absolute path
a9d7379 
Rationale: we accept:

    summon -f /abs/path/secrets.yml

but not:

    summon -f /abs/path/secrets.yml -u

since it doesn't make sense to recurse looking for something that is
absolute

Signed-off-by: Marco Molteni <[email protected]>
@marco-m
Copy link
Contributor Author

marco-m commented Mar 20, 2020

@izgeri added the sign-off tag.

BradleyBoutcher
BradleyBoutcher suggested changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@BradleyBoutcher BradleyBoutcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work overall. I really appreciate how thorough your testing is and the clarity of your code. There's a few small adjustments that need to be made, but this is a great contribution!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sgnn7
sgnn7 suggested changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marco-m Added a few comments as well. Can you also make sure to update the changelog too?

README.md Outdated Show resolved Hide resolved
internal/command/action.go Outdated Show resolved Hide resolved
internal/command/action.go Show resolved Hide resolved
internal/command/action.go Show resolved Hide resolved
internal/command/action_test.go Outdated Show resolved Hide resolved
internal/command/action_test.go Outdated Show resolved Hide resolved
internal/command/action_test.go Outdated Show resolved Hide resolved
internal/command/action_test.go Outdated Show resolved Hide resolved
internal/command/action_test.go Outdated Show resolved Hide resolved
internal/command/flags.go Show resolved Hide resolved
@marco-m
Address review comments
0fbfc84 
Signed-off-by: Marco Molteni <[email protected]>
@marco-m
Copy link
Contributor Author

marco-m commented Mar 24, 2020 •
edited
Loading

@sgnn7 I addressed all the points but two, and left two questions for you.

@sgnn7
Copy link
Contributor

sgnn7 commented Mar 25, 2020

@marco-m Re-replied! Thank you for the patience as we get this through our process!

@marco-m
Copy link
Contributor Author

marco-m commented Mar 25, 2020

I addressed the 2 remaining discussion points. I am surprised by the codeclimate checks, they should have failed also before?

@sgnn7
Copy link
Contributor

sgnn7 commented Mar 25, 2020

@marco-m The CodeClimate issues have been cleared on our side - we just added the configuration for it and we still have some cleanup to do on it.

sgnn7
sgnn7 approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marco-m LGTM! I'll just check w/ @BradleyBoutcher to see if he wants to make any other notes on this.

@sgnn7 sgnn7 merged commit 36c7536 into cyberark:master Mar 25, 2020
@sgnn7
Copy link
Contributor

sgnn7 commented Mar 25, 2020

@marco-m Thank you for the contrib! We will bundle this into a release at some point and I'll ping you when we do.

@marco-m marco-m deleted the recurse-upwards branch March 25, 2020 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BradleyBoutcher BradleyBoutcher BradleyBoutcher approved these changes

@sgnn7 sgnn7 sgnn7 approved these changes

Assignees
No one assigned
Labels
contributor
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to traverse up the directory hierarchy looking for secrets.yml
4 participants
@marco-m @izgeri @sgnn7 @BradleyBoutcher