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

[FEATURE] Support doppler secrets set --visibility #454

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

aisrael
Copy link
Contributor

@aisrael aisrael commented Mar 25, 2024

Why was this pull request opened?

Submitting this as a proof-of-concept and potential implementation for [FEATURE] Set secret visibility #453

What was changed?

  • Mainly, the secrets.setSecrets() function was modified to compose an array of model.ChangeRequest instead of using a map[string]interface{}
  • This also required modifying the model.ChangeRequest to add all fields in the secrets-update API

How to test?

$ go run . secrets set -p test -c test TEST=value --visibility masked
┌──────┬───────┬──────┐
│ NAME │ VALUE │ NOTE │
├──────┼───────┼──────┤
│ TEST │ value │      │
└──────┴───────┴──────┘

$ go run . secrets set -p test -c test TEST=value --visibility restricted
┌──────┬──────────────┬──────┐
│ NAME │ VALUE        │ NOTE │
├──────┼──────────────┼──────┤
│ TEST │ [RESTRICTED] │      │
└──────┴──────────────┴──────┘

@aisrael aisrael requested a review from a team as a code owner March 25, 2024 16:17
@nmanoogian
Copy link
Member

Thanks for this recommendation and implementation, @aisrael! We'll take a look and provide any necessary feedback.

@watsonian
Copy link
Contributor

This is looking good from my testing! Setting secrets with secrets set and via the tui are working as expected. @aisrael I would recommend the following change though:

diff --git a/pkg/cmd/secrets.go b/pkg/cmd/secrets.go
index 342fa8c..3a04d16 100644
--- a/pkg/cmd/secrets.go
+++ b/pkg/cmd/secrets.go
@@ -237,6 +237,7 @@ func setSecrets(cmd *cobra.Command, args []string) {
 	canPromptUser := !utils.GetBoolFlag(cmd, "no-interactive")
 	localConfig := configuration.LocalConfig(cmd)
 	visibility := cmd.Flag("visibility").Value.String()
+	visibilityModified := visibility != ""
 
 	utils.RequireValue("token", localConfig.Token.Value)
 
@@ -314,7 +315,7 @@ func setSecrets(cmd *cobra.Command, args []string) {
 			Name:  key,
 			Value: &value,
 		}
-		if visibility != "" {
+		if visibilityModified {
 			changeRequest.Visibility = &visibility
 		}
 		changeRequests = append(changeRequests, changeRequest)
@@ -327,7 +328,7 @@ func setSecrets(cmd *cobra.Command, args []string) {
 			Name:  key,
 			Value: &value,
 		}
-		if visibility != "" {
+		if visibilityModified {
 			changeRequest.Visibility = &visibility
 		}
 		changeRequests = append(changeRequests, changeRequest)
@@ -347,7 +348,7 @@ func setSecrets(cmd *cobra.Command, args []string) {
 			} else {
 				changeRequest.Value = &secretArr[1]
 			}
-			if visibility != "" {
+			if visibilityModified {
 				changeRequest.Visibility = &visibility
 			}
 			changeRequests = append(changeRequests, changeRequest)
@@ -360,7 +361,7 @@ func setSecrets(cmd *cobra.Command, args []string) {
 	}
 
 	if !utils.Silent {
-		printer.Secrets(response, keys, jsonFlag, false, raw, false, false)
+		printer.Secrets(response, keys, jsonFlag, false, raw, false, visibilityModified)
 	}
 }

This will make it so if the visibility is adjusted the table that gets printed afterward will include the visibility values as well.

@aisrael
Copy link
Contributor Author

aisrael commented Mar 28, 2024

@watsonian Made the changes as suggested.

@watsonian
Copy link
Contributor

@aisrael Awesome! We'll need another engineer on our CLI team to review before approving for merge, but it's looking good!

@nmanoogian
Copy link
Member

@aisrael This is looking great! It looks like we had a failing test as a result of an earlier change. Would you mind rebasing your branch on DopplerHQ:master?

@aisrael
Copy link
Contributor Author

aisrael commented Mar 28, 2024

@nmanoogian Just updated my fork/branch from DopplerHQ/cli:master

@nmanoogian
Copy link
Member

@aisrael Excellent! Sorry to do this but one more ask that I forgot about. Can you prepend chore: to your "Show visibility" commit message (similar to 3f12e95). We autogenerate our release notes via the commit message and prepending chore: keeps the noise down.

@aisrael
Copy link
Contributor Author

aisrael commented Mar 29, 2024

So... should I just push a 'dummy' commit in my fork branch with chore:?

@watsonian
Copy link
Contributor

@aisrael I believe all Nic is looking for is a git rebase -i where you adjust the commit message on your initial commit (you can get rid of the "dummy" commit you made). In fact, you could probably just roll all your commits into a single message and then force-push it up on this branch.

@aisrael
Copy link
Contributor Author

aisrael commented Mar 29, 2024

Got it, ok will try

@nmanoogian nmanoogian merged commit 6d63650 into DopplerHQ:master Apr 3, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants