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

OSSM-1188 Remove ServiceMeshExtension support #1016

Merged
merged 1 commit into from Oct 4, 2022

Conversation

dgn
Copy link
Contributor

@dgn dgn commented Sep 28, 2022

This removes the wasm-cache chart from the 2.3 helm charts. It also adds upgrade validation: upgrading to v2.3 will fail if you have existing SMEs in your member namespaces. This check however is only performed when upgrading, not when creating new SMCPs, reasons being:

  • when you're recreating an SMCP, you probably don't expect an existing SME to "still work". It's a new deployment after all.
  • the check is a little costly, so it makes sense to only run it when absolutely necessary

pkg/controller/versions/strategy_v2_3.go Outdated Show resolved Hide resolved
if err := cl.List(ctx, serviceMeshExtensions); err != nil {
fmt.Println(err)
if !errors.IsNotFound(err) {
logger.Error(err, "failed to list ServiceMeshExtensions")
Copy link
Member

Choose a reason for hiding this comment

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

it failed because there's no SME CR, so, it's expected and not an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what you mean? the test passes locally

Copy link
Member

Choose a reason for hiding this comment

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

I mean: trying to list a SME and get a failure should not be logged as an error, should it? If there's no SME CRD in the cluster, for example, an error is expected and desirable, right?

Copy link
Member

Choose a reason for hiding this comment

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

Also, there's a println hidden in there :D

Copy link
Contributor Author

@dgn dgn Sep 28, 2022

Choose a reason for hiding this comment

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

I mean: trying to list a SME and get a failure should not be logged as an error, should it? If there's no SME CRD in the cluster, for example, an error is expected and desirable, right?

Hmm in that case you'd be right. We will have to still install the CRD though, as a user might install a pre-2.3 version - and we always install the same set of CRDs for all control plane versions. But I can check for that error

Also, there's a println hidden in there :D

not anymore, I removed that 😉 it's just still visible here for some reason

pkg/controller/versions/strategy_v2_3.go Outdated Show resolved Hide resolved
@dgn dgn force-pushed the OSSM-1188 branch 3 times, most recently from 9e6284b to 939771e Compare September 29, 2022 10:05
@dgn dgn force-pushed the OSSM-1188 branch 2 times, most recently from 2857ce5 to 218babe Compare October 4, 2022 09:29
@maistra-bot maistra-bot merged commit 72c16b2 into maistra:maistra-2.3 Oct 4, 2022
@dgn dgn deleted the OSSM-1188 branch October 4, 2022 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants