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

Upgrade operator sdk 125 #240

Merged
merged 3 commits into from
Feb 2, 2023
Merged

Conversation

littlejawa
Copy link
Contributor

Following steps from https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.25.0/

This PR will require more time, mostly because I'm not sure about the consequences of the changes I'm making.

Enable multi-arch build support

I have added the code related to that in the Makefile, but my understanding is that this is just enabling it if we ever want to support more archs. It requires additional changes to actually build anything.
I'm not sure if/how we can test that change without the appropriate support, and I don't know if we actually want/can support other archs... so I'm leaving it here for discussion.

Update ginkgo to v2

Updating the dependency in go.mod is reverted by the next go mod tidy call if we don't actually use that version of ginkgo in our tests.
Moving to ginkgo v2 is not a simple step. It has its own migration guide to follow :
https://onsi.github.io/ginkgo/MIGRATING_TO_V2

According to this, the v1 is deprecated so it really seems we have to update.

Doing it reveals two issues in the suite_test.go file:

  • BeforeSuite doesn't use the done parameter anymore, nor a timeout value, effectively making it synchronous.
    This is on purpose according to https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing
    The migration guide provides guidance on how to make it async again if we really need to, using different methods, but I didn't go that far.
    I don't think we actually have parallel tests running anyway, and the execution time of make test on main and on this PR branch are the same.
    I feel we can leave it that way, but tell me if you think otherwise, and we can try to change it.

  • RunSpecsWithDefaultAndCustomReporters is deprecated
    See https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-custom-reporters
    The call is then made without the NewlineReporter... but I admit I have no idea what the consequences are.
    Where can I check for the output ? I see nothing from make test on the main branch already.
    The migration guide says you should use an additional parameter to ginkgo to get some reports, but I don't see ginkgo being called anywhere actually (we just use "go test"). Is that where we should add it? Do we need it?
    I tried to use the ReportAfterSuite method from the migration guide, but the NewlineReporter is not accepted there, and I can't really test it if I don't know where to look for the output. Some guidance would be appreciated.

I kept the ginkgo changes in a separate commit so that we can modify this part separately if needed.

misc

  • the chmod call in the makefile was added because the modification to KUBEBUILDER_ASSETS creates a folder under bin/k8s/ that cannot be deleted by make clean otherwise. I kept the commit separated in case a better method is suggested for fixing it.
  • I updated all the way up to 1.25.3, because there is no change for 1.25.1, 1.25.2 and 1.25.3.

…sdk-version/v1.25.0/

Not updating ginkgo to v2 yet - this will require further changes.

Signed-off-by: Julien Ropé <[email protected]>
Following the guide from https://onsi.github.io/ginkgo/MIGRATING_TO_V2

Signed-off-by: Julien Ropé <[email protected]>
@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 Dec 8, 2022
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 8, 2022
@openshift-ci
Copy link

openshift-ci bot commented Dec 8, 2022

Hi @littlejawa. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cpmeadors
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 8, 2022
Copy link
Contributor

@cpmeadors cpmeadors left a comment

Choose a reason for hiding this comment

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

Multiarch support looks additive and shouldn't change affect our downstream builds. ginkgov2 changes look good and nice to see. Everything else looks good

@openshift-ci
Copy link

openshift-ci bot commented Dec 8, 2022

@littlejawa: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sandboxed-containers-operator-e2e 32be357 link false /test sandboxed-containers-operator-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@littlejawa littlejawa changed the title [WIP] Upgrade operator sdk 125 Upgrade operator sdk 125 Dec 19, 2022
@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 Dec 19, 2022
Copy link
Contributor

@jensfr jensfr left a comment

Choose a reason for hiding this comment

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

lgtm, thank you @littlejawa

@jensfr jensfr merged commit 2a86bbf into openshift:main Feb 2, 2023
@littlejawa littlejawa deleted the upgrade_operator_sdk_125 branch February 3, 2023 09:08
gkurz added a commit to gkurz/sandboxed-containers-operator that referenced this pull request Jul 15, 2024
`make test` is known to be failing since a long time. An issue is that
it thus leaks a directory that prevents `make clean` to succeed because
of restrictive file access modes.

Separate the building of the test, which introduces the file access mode
issue, from the actual test run. This allows to fully benefit from the
access mode fixing introduced by PR openshift#240.

Signed-off-by: Greg Kurz <[email protected]>
beraldoleal pushed a commit to beraldoleal/sandboxed-containers-operator that referenced this pull request Aug 7, 2024
`make test` is known to be failing since a long time. An issue is that
it thus leaks a directory that prevents `make clean` to succeed because
of restrictive file access modes.

Separate the building of the test, which introduces the file access mode
issue, from the actual test run. This allows to fully benefit from the
access mode fixing introduced by PR openshift#240.

Signed-off-by: Greg Kurz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants