-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add support to secrets #449
Conversation
- GetSecrets - CreateOrUpdateDeploySecretEnvVar - CreateOrUpdateCronJobSecretEnvVar
- SetSecret - UnsetSecret
Codecov Report
@@ Coverage Diff @@
## master #449 +/- ##
==========================================
- Coverage 49.64% 49.38% -0.27%
==========================================
Files 63 63
Lines 4083 4299 +216
==========================================
+ Hits 2027 2123 +96
- Misses 1913 2019 +106
- Partials 143 157 +14
Continue to review full report at Codecov.
|
There's a way to "hack" this problem, according to this comment, we can change (or create) an annotation inside of the spec of deploy (spec.template.metadata.annotations) with a random value or the current date (doesn't matter). |
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.
LGTM
pkg/server/app/app_test.go
Outdated
ops.(*AppOperations).kops = &errK8sOperations{ | ||
CreateOrUpdateDeploySecretEnvVarsErr: ErrInvalidEnvVarName, | ||
GetSecretErr: ErrNotFound, | ||
NamespaceErr: nil, |
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.
I guess NamespaceErr
is already nil, right?
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.
Yes, my fault
pkg/client/cmd/app.go
Outdated
if _, err := cli.SetSecret(context.Background(), req); err != nil { | ||
client.PrintErrorAndExit(client.GetErrorMsg(err)) | ||
} | ||
fmt.Println("Env vars updated with success") |
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.
maybe Secrets updated? As a secret is exposed as env var I don't know which is best
@@ -712,9 +861,15 @@ func init() { | |||
|
|||
appEnvSetCmd.Flags().String("app", "", "app name") | |||
appEnvSetCmd.Flags().Bool("no-input", false, "set env vars without warning") | |||
// App unset env vars |
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.
👏
CHANGELOG.md
Outdated
- support to filter app logs by container name | ||
- Support for internal apps | ||
- Support to filter app logs by container name | ||
- Support to Secrets as env var |
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.
Maybe Support to set secrets as env vars
currentClusterName := cfgCluster | ||
if currentClusterName == "" { | ||
var err error |
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.
🤔 why?
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.
🤔
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.
to solve go build
issue:
pkg/client/cmd/app.go|418| 30: currentClusterName declared and not used
😢
For me there's no problem in always doing a rolling update. |
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.
LGTM
currentClusterName := cfgCluster | ||
if currentClusterName == "" { | ||
var err error |
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.
🤔
@@ -784,6 +796,35 @@ func (c *Client) patchCronJobEnvVars(namespace, name string, v interface{}) erro | |||
return errors.Wrap(err, "patch cronjob failed") | |||
} | |||
|
|||
func convertAppSecretEnvVar(secretName string, secrets []string) interface{} { |
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.
Maybe put this on converters.go
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.
I'm not sure, converters.go
is for teresa struct -> k8s struct
Agree with @aguerra |
Thanks @aguerra and @yagonobre I'll add the "fake" annotation to forces the rolling update |
This change forces an rolling update on each [env|secret]-[un]set command
@aguerra @yagonobre UPDATED |
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.
🔁
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.
🔁
There is a problem, change one secret that already exists doesn't perform the rolling update (because the deploy spec doesn't change).