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

🌱 Use server side apply to update test resource #791

Merged

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented Apr 26, 2024

Description

Deals with a flake such as this:

go test -tags upstream -v ./test/e2e/...
=== RUN   TestClusterExtensionPackageUniqueness
    cluster_extension_admission_test.go:30: create first resource
    cluster_extension_admission_test.go:42: create second resource with the same package as the first resource
    cluster_extension_admission_test.go:54: create second resource with different package
    cluster_extension_admission_test.go:66: update second resource with package which already exists on the cluster
    cluster_extension_admission_test.go:69: 
        	Error Trace:	/home/runner/work/operator-controller/operator-controller/test/e2e/cluster_extension_admission_test.go:69
        	Error:      	Error "Operation cannot be fulfilled on clusterextensions.olm.operatorframework.io \"test-extension-74hdj\": the object has been modified; please apply your changes to the latest version and try again" does not contain "Package \"package1\" is already installed via ClusterExtension \"test-extension-first\""
        	Test:       	TestClusterExtensionPackageUniqueness
--- FAIL: TestClusterExtensionPackageUniqueness (2.03s)

Was introduced in #785.

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2024
Copy link

netlify bot commented Apr 26, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit ab7ddfa
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/662bb58d8376550008264509
😎 Deploy Preview https://deploy-preview-791--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@m1kola m1kola marked this pull request as ready for review April 26, 2024 14:10
@m1kola m1kola requested a review from a team as a code owner April 26, 2024 14:10
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2024
@m1kola m1kola requested a review from perdasilva April 26, 2024 15:16

ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
)

func TestClusterExtensionPackageUniqueness(t *testing.T) {
ctx := context.Background()
fieldOwner := client.FieldOwner("operator-controller-e2e")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using SSA is valid and maybe we should have a wider discussion about how best to use it in e2e tests. But, we shouldn't be seeing conflicts of the type in the issue coming out of our test code. If we do it's because we forgot to get the latest from the server after a create or update. SSA is nice because it makes things feel more natural for e2e tests. Do we need to respect any conventions when using SSA? Should we switch all mutating interactions in e2e over to SSA?

Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

lgtm - on a general note we may want to consider best practices (if any) around SSA that we should respect in the e2es

@m1kola m1kola added this pull request to the merge queue Apr 29, 2024
Merged via the queue into operator-framework:main with commit e672804 Apr 29, 2024
15 checks passed
@m1kola m1kola deleted the flake_cluster_extension_admission_test branch June 14, 2024 14:40
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.

None yet

2 participants