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.

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

cmd/sops: use --config in subcommands #559

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

q3k
Copy link

@q3k q3k commented Oct 29, 2019

Currently that flag is not being used by subcommands (publish and updatekeys). This fixes that problem.

I'm not exactly sure how to go about testing this. Any ideas?

@ajvb
Copy link
Contributor

ajvb commented Oct 29, 2019

@q3k Oh nice. Thank you for the patch! Would you mind pointing this PR at our develop branch?

In regards to testing, at the moment it looks like we don't have any functional tests for this, so if you'd like to add some that'd be great. Otherwise, I'd build this locally and confirm that it works with updatekeys and regular encrypt/decrypt options for now. It'll go through some testing before it's released.

@ajvb ajvb requested review from autrilla and ajvb and removed request for autrilla October 29, 2019 19:11
@q3k q3k force-pushed the q3k/fix-config-flag branch from 3829479 to abea047 Compare October 29, 2019 19:50
@q3k q3k changed the base branch from master to develop October 29, 2019 19:50
@q3k
Copy link
Author

q3k commented Oct 29, 2019

Just rebased against develop.

I would appreciate if you tested it locally for now, I might take a stab at some tests tomorrow (and either update this PR or send off a new one), but I can't promise anything.

cmd/sops/main.go Outdated
@@ -960,13 +960,22 @@ func keyGroups(c *cli.Context, file string) ([]sops.KeyGroup, error) {
return []sops.KeyGroup{group}, nil
}

// getConfigPath returns the config path that should be used, either by finding it
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the current phrasing is a bit confusing and could lead people to believe that it will first find from a given root, and it that fails it will use the one passed through the command line, when it's actually the opposite

@lucendio
Copy link

lucendio commented Feb 6, 2020

Is there anything else that is needed to get this merged?

/cc @ajvb

@ajvb
Copy link
Contributor

ajvb commented Feb 11, 2020

@lucendio Yes, @autrilla's comment should be addressed and needs to be tested.

@flokli
Copy link

flokli commented Jun 8, 2020

@q3k can you address the review comment and rebase this PR?

@q3k
Copy link
Author

q3k commented Jun 21, 2020

@flokli Apologies! I'll do this now.

@q3k q3k force-pushed the q3k/fix-config-flag branch from abea047 to 8740120 Compare June 21, 2020 18:56
@flokli
Copy link

flokli commented Aug 4, 2020

This needs yet another rebase unfortunately.

@ajvb ajvb added this to the v3.7.3 milestone Mar 3, 2022
@ajvb ajvb modified the milestones: v3.7.3, v3.8.0 May 2, 2022
@felixfontein
Copy link
Contributor

@q3k would you mind rebasing another time, and signing off your commits? It would be really great to get this finally merged.

@felixfontein
Copy link
Contributor

updatekeys has already been fixed in #672, though that fix no longer works. #1613 is there to fix it again. publish doesn't support the flag yet, though.

@felixfontein
Copy link
Contributor

I re-created the missing part in #1779.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants