-
Notifications
You must be signed in to change notification settings - Fork 900
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
Match file path relative to config file's directory #827
Match file path relative to config file's directory #827
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR. Before feeling comfortable merging this, I would like to see the documentation mention how the creation rules path is computed, and some tests as well.
Hey @autrilla, please tell me if it's ok now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
Oh, can you make this merge against |
Rebased to develop! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one thing I missed in the original review, and then we're good to go.
config/config.go
Outdated
@@ -65,6 +67,7 @@ func FindConfigFile(start string) (string, error) { | |||
type configFile struct { | |||
CreationRules []creationRule `yaml:"creation_rules"` | |||
DestinationRules []destinationRule `yaml:"destination_rules"` | |||
Path string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this in the last review, but this struct is used to parse the .sops.yaml file, and I don't want this exposed in there. Can you move this out of the struct and just pass it in as a parameter to the functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Tell me if it's ok now.
@autrilla I just saw you reverted the merge. Was that because of the failing tests (I just saw that as well). Do you have anything against merging a new PR (with passing tests)? |
Yes, it was because of failing tests after merging. Nothing against merging
a new PR that merges with passing tests.
…On Fri, 9 Apr 2021 at 23:30, Paulo Lieuthier ***@***.***> wrote:
@autrilla <https://github.com/autrilla> I just saw you reverted the
merge. Was that because of the failing tests (I just saw that as well). Do
you have anything against merging a new PR (with passing tests)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#827 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARH4V2ZAVDMTDWXVJUHT4TTH5WXBANCNFSM4YRYLXAQ>
.
|
Fixes #480.