cli: Create Secrets With A Label #835
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ishustava
reviewed
Nov 5, 2021
There was a problem hiding this comment.
Great work, Saad!
A couple of comments/questions:
- Should we also add label if the secret exists? I think some commands already update the secret if it exists (e.g webhook-cert-manager), while others (like
server-acl-init) don't. Adding a label if secret exists is nice because if someone runs a helm upgrade, all secrets would get updated with a label. - I think the commands need unit tests to check that the label is added.
t-eckert
reviewed
Nov 5, 2021
t-eckert
reviewed
Nov 5, 2021
t-eckert
reviewed
Nov 5, 2021
t-eckert
reviewed
Nov 5, 2021
There was a problem hiding this comment.
Great work! Very clear code. I would echo what @ishustava said and change the "consul-k8s" string in both the CLI and Control Plane to be a constant that is passed around. That will make it easier for us to change later on if we need to.
sadjamz
force-pushed
the
cli-managed-secrets
branch
2 times, most recently
from
1a71d07
to
cb12fe9
Compare
ishustava
approved these changes
Nov 18, 2021
sadjamz
force-pushed
the
cli-managed-secrets
branch
from
08f0204
to
2046c56
Compare
t-eckert
reviewed
Nov 22, 2021
t-eckert
reviewed
Nov 22, 2021
Comment on lines
395
to
+400
| for _, secret := range secrets.Items { | ||
| if strings.HasPrefix(secret.Name, foundReleaseName) { | ||
| err := c.kubernetes.CoreV1().Secrets(foundReleaseNamespace).Delete(c.Ctx, secret.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("deleteSecrets: error deleting Secret %q: %s", secret.Name, err) | ||
| } | ||
| secretNames = append(secretNames, secret.Name) | ||
| err := c.kubernetes.CoreV1().Secrets(foundReleaseNamespace).Delete(c.Ctx, secret.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("deleteSecrets: error deleting Secret %q: %s", secret.Name, err) | ||
| } | ||
| secretNames = append(secretNames, secret.Name) |
There was a problem hiding this comment.
I would move the instantiation of secretNames to right above line 394, before you start the iteration. I would also change the name to deletedSecrets so it's more clear what you are listing.
There was a problem hiding this comment.
I lean towards secretNames rather than changing it to deletedSecrets because they are the names of secrets to delete, but haven't been deleted yet.
t-eckert
reviewed
Nov 22, 2021
t-eckert
approved these changes
Nov 22, 2021
ndhanushkodi
force-pushed
the
cli-managed-secrets
branch
from
bdc4a32
to
85fcdcf
Compare
… tests as well) and only delete secrets with that label in uninstall.go
…troduced a regression while resolving merge conflict.
Co-authored-by: Thomas Eckert <teckert@hashicorp.com>
ndhanushkodi
force-pushed
the
cli-managed-secrets
branch
from
6e1b28a
to
07d64ef
Compare
This reverts commit 07d64ef.
ndhanushkodi
added a commit
that referenced
this pull request
Dec 3, 2021
This reverts commit e895995.
ndhanushkodi
added a commit
that referenced
this pull request
Dec 3, 2021
This reverts commit e895995.
ndhanushkodi
added a commit
that referenced
this pull request
Dec 3, 2021
else. the control plane changes are in the image which were fine and passed the previous cli acceptance tests. cli: Create Secrets With A Label (#835) Every-time a secret is created, add a label "managed-by" which is equal to "consul-k8s". On uninstall, only delete those secrets with said label. Co-authored-by: Thomas Eckert <teckert@hashicorp.com> Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>
ndhanushkodi
added a commit
that referenced
this pull request
Dec 6, 2021
The cloud acceptance tests for the CLI have been failing (but passing in Kind) since merging 835. After reverting the whole PR and testing with several parts of that PR, it looks like the code itself wasn't causing issues, but rather the extensive go.mod updates. Those updates were not necessary for the PR (I was able to run go mod tidy with no changes), so I'm reverting those here so the tests will pass again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
How I've tested this PR:
Changed the uninstall test, and ensured any secret created with this label is deleted, while secrets lacking this label persist. Also some ad-hoc testing, running the
consul-k8s installandconsul-k8s uninstallcommand.How I expect reviewers to test this PR:
Code review.
Checklist: