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

Add chartmigration resource and disable chartconfig controller #358

Merged
merged 10 commits into from
Mar 9, 2020

Conversation

rossf7
Copy link
Contributor

@rossf7 rossf7 commented Feb 28, 2020

Towards giantswarm/roadmap#30

Now that we only use chart CRs in new tenant clusters the only purpose of the legacy chartconfig controller is for deleting chartconfig CRs.

This is now done by a new chartmigration resource. It checks for the chart-operator.giantswarm.io/delete-custom-resource-only annotation which is added by cluster-operator once the migration is complete.

https://github.com/giantswarm/cluster-operator/blob/legacy/service/controller/resource/appmigration/create.go#L150-L155

@rossf7 rossf7 self-assigned this Feb 28, 2020
@rossf7 rossf7 marked this pull request as ready for review February 28, 2020 13:40
E2E_TEST_DIR: "integration/test/chartconfig/chartvalues"
E2E_TESTED_VERSION: "current"
<<: *e2eTest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

chartconfig tests get removed.

chartConfig, err := r.g8sClient.CoreV1alpha1().ChartConfigs(cr.Namespace).Get(cr.Name, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
r.logger.LogCtx(ctx, "level", "debug", "message", fmt.Sprintf("did not find chartconfig %#q. nothing to do.", cr.Name))
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no chartconfig CR we can stop here.

finalizers := []string{}

for _, f := range chartConfig.ObjectMeta.Finalizers {
if f == "operatorkit.giantswarm.io/chart-operator-chartconfig" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the annotation is in place we only remove the operatorkit finalizer.

@rossf7 rossf7 requested review from a team February 28, 2020 14:28
}

chartConfig.ObjectMeta.Finalizers = finalizers
_, err = r.g8sClient.CoreV1alpha1().ChartConfigs("giantswarm").Update(chartConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simply use patch operation to remove specific finalizers? I think we did many times before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True good point. I'll update to do that.

I also found a problem with getting the chartconfig CR. Some of the CR names are different as they have kubernetes- prefixes.

But they have the same app label so I'll use that instead.

@rossf7
Copy link
Contributor Author

rossf7 commented Feb 28, 2020

Changes made. Tested on ghost.

@rossf7 rossf7 mentioned this pull request Mar 6, 2020
4 tasks
@rossf7
Copy link
Contributor Author

rossf7 commented Mar 9, 2020

@tomahawk28 PTAL. Doing a final test but this should be OK now.

Copy link
Contributor

@tomahawk28 tomahawk28 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for a PR :)

@rossf7
Copy link
Contributor Author

rossf7 commented Mar 9, 2020

Tested on ghost. Going with this.

@rossf7 rossf7 merged commit 0ead9c4 into master Mar 9, 2020
@rossf7 rossf7 deleted the chartmigration-resource branch March 9, 2020 13:10
mproffitt pushed a commit that referenced this pull request Aug 11, 2022
* Disable chartconfig integration tests

* Remove chartconfig controller

* Add chartmigration resource

* Get chartconfig CR from its app label and remove finalizer using JSON patch

* Add app labels to migration test
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.

2 participants