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

MigCluster : Use generateName to create unique secrets #759

Merged
merged 10 commits into from Mar 23, 2020

Conversation

pranavgaikwad
Copy link
Contributor

Fixes #396

Proposed Changes :
[1] Creating new MigCluster

  • We use generateName instead of name in tokenSecret of MigCluster. We also introduce two new labels on the Secret : createdForResourceType, createdForResource. This makes it easier to find a secret associated with a cluster.
  • We create secret first, then get the resulting secret name, then we create MigCluster

[2] Deleting MigCluster

  • We find the secret name before deleting MigCluster
  • Then usual flow of delete

[3] Editing MigCluster

  • Nothing changes here except the secret reference.

@pranavgaikwad pranavgaikwad requested a review from a team March 20, 2020 13:35
@@ -304,7 +327,7 @@ function* updateClusterRequest(action) {

// Pushing a request fn to delay the call until its yielded in a batch at same time
updatePromises.push(() => client.patch(
secretResource, clusterValues.name, newTokenSecret));
secretResource, currentCluster.spec.serviceAccountSecretRef.name, newTokenSecret));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I am assuming that Cluster name is not changeable from UI

Copy link
Contributor

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

Remove cluster is currently broken with this PR This was a result of deleting a cluster without the associated changes in this PR. Works fine if you add & delete cluster with the updated code.

@pranavgaikwad
Copy link
Contributor Author

@ibolton336 added a fix to address the problem you saw. safe guarded the case when - associated Secret does not exist and we try to delete a cluster.

Copy link
Contributor

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

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

Just needs some code formatting changes for ternary consistency. Otherwise LGTM

src/app/cluster/duck/sagas.ts Outdated Show resolved Hide resolved
src/app/cluster/duck/sagas.ts Outdated Show resolved Hide resolved
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.

Clusters and Storage sharing the same name will have secrets fail to create
2 participants