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

THREESCALE-11281 - bump to go 1.22 #1036

Closed
wants to merge 1 commit into from

Conversation

valerymo
Copy link
Contributor

@valerymo valerymo commented Nov 27, 2024

Jira: https://issues.redhat.com/browse/THREESCALE-11281

Verification

  1. Install and sanity validation
    3scale installed locally
~/go/3scale-operator git branch --show-current
THREESCALE-11281-1


cd 3scale-operator
make install
export NAMESPACE=3scale-test
oc new-project $NAMESPACE
oc project $NAMESPACE
make download
make run

Installed correctly
routes/UI verified

~/go/3scale-operator oc project
Using project "3scale-test" on server "https://api.vmo01.xxxxx".
~/go/3scale-operator date                     
Wed Nov 27 19:17:24 EET 2024
~/go/3scale-operator git branch --show-current
THREESCALE-11281-1
~/go/3scale-operator oc get deploy            
NAME                 READY   UP-TO-DATE   AVAILABLE   AGE
apicast-production   1/1     1            1           36m
apicast-staging      1/1     1            1           36m
backend-cron         1/1     1            1           40m
backend-listener     1/1     1            1           40m
backend-redis        1/1     1            1           40m
backend-worker       1/1     1            1           40m
system-app           1/1     1            1           34m
system-memcache      1/1     1            1           40m
system-mysql         1/1     1            1           40m
system-redis         1/1     1            1           40m
system-searchd       1/1     1            1           40m
system-sidekiq       1/1     1            1           36m
zync                 1/1     1            1           36m
zync-database        1/1     1            1           36m
zync-que             1/1     1            1           36m
~/go/3scale-operator 


  1. CVEs analysys
    Quay.io Security Scan - Passed
  • Image was built to see CVE check in Quay.io.
  • Repo: quay.io/vmogilev_rhmi/3scale-operator:test-go122
  • Command used to build image (on Mac arm):
docker build . --platform linux/amd64 -t quay.io/vmogilev_rhmi/3scale-operator:test-go122

image

image

@valerymo valerymo requested a review from a team as a code owner November 27, 2024 10:25
@valerymo valerymo force-pushed the THREESCALE-11281-1 branch 9 times, most recently from 0260d86 to 3326fa3 Compare November 27, 2024 15:03
@valerymo valerymo changed the title [WIP]THREESCALE-11281 - bump to go 1.22 THREESCALE-11281 - bump to go 1.22 Nov 27, 2024
PROJECT Outdated
- group: apps
-
# TODO(user): Uncomment the below line if this resource implements a controller, else delete it.
# controller: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be uncommented right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Thank you

PROJECT Outdated
- group: apps
-
# TODO(user): Uncomment the below line if this resource implements a controller, else delete it.
# controller: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This too? Can you review this file again please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Thank you

PROJECT Outdated
kind: APIManager
# TODO(user): Update the package path for your API if the below value is incorrect.
path: k8s.io/api/apps/v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. Thank you

@@ -281,6 +286,9 @@ bundle-validate: $(OPERATOR_SDK)

.PHONY: bundle-update-test
bundle-update-test:
# Remove the createdAt field before checking the diff
sed -i '/createdAt:/d' ./bundle/manifests/3scale-operator.clusterserviceversion.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks hacky Valery, we shouldn't do that because essentially the bundle will be created in our build system and that flag will be there. Is there no other way around it? (I'll try finding a different approach as well)

Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove the createdAt from the config/manifest/bases/3scale-opreator.clusterserviceversion.yaml
The flag is updated if present, if not, it's not.

@@ -1161,4 +1166,25 @@ spec:
maturity: stable
provider:
name: Red Hat
relatedImages:
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to:

  • figure out where this is coming from / why?
  • redis and psql have no "name" values - why
  • check midstream for possible clash when updating these values for prod release.

@@ -240,12 +240,13 @@ metadata:
categories: Integration & Delivery
certified: "false"
containerImage: quay.io/3scale/3scale-operator:master
createdAt: "2019-05-30T22:40:00Z"
createdAt: "2024-11-27T13:51:25Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

as above, if we remove it from base we won't have it, I don't think this is a mandatory field for csv validation. Please remove

@@ -2,7 +2,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.9.2
controller-gen.kubebuilder.io/version: v0.14.0
Copy link
Contributor

Choose a reason for hiding this comment

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

@carlkyrillos was there something about this bump that you had an issue with in one of your previous PRs?

Copy link
Contributor Author

@valerymo valerymo Dec 17, 2024

Choose a reason for hiding this comment

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

controller-gen v0.9.2 has a problem with GO 1.22 - following steps failed: assets-validate, bundle-validate, run-unit-tests, test-crds. All with same/similar error in controller-gen - segmentation fault , as for example

/home/circleci/project/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa00d6f]

Looks like known Issue - kubernetes-sigs/controller-tools#880 . People using newer controller-gen 0.14 . Same version used also in marin3r.

@MStokluska
Copy link
Contributor

MStokluska commented Dec 11, 2024

@valerymo left some comments, in general, could you explain to me why are we pinning operator-sdk bump to go bump?
If operator SDK bump is really required, we should follow (or at least review) the migration steps in go operators here:
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.28.0/ - that said, I think stuff like "controllers should now be in internal/controllers" could potentially be skipped, but it would be good to review the migration required from 1.2 to 1.28 just to be extra cautious...

@valerymo
Copy link
Contributor Author

valerymo commented Dec 16, 2024

@valerymo left some comments, in general, could you explain to me why are we pinning operator-sdk bump to go bump? If operator SDK bump is really required, we should follow (or at least review) the migration steps in go operators here: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.28.0/ - that said, I think stuff like "controllers should now be in internal/controllers" could potentially be skipped, but it would be good to review the migration required from 1.2 to 1.28 just to be extra cautious...

@MStokluska , I had problems using go 1.22 with operator-sdk 1.2. So the question was - which verion of operator-sdk to migrage. Here’s a summary of the versions and their compatibility with Go 1.22.

Go Version Operator SDK Version Notes
Go 1.18 - 1.20 Operator SDK 1.2 Works well with Go 1.18 and 1.19, but may cause issues with Go 1.22 due to Go modules changes.
Go 1.21 - 1.22 Operator SDK 1.28+ Fully compatible with Go 1.21 and 1.22. Recommended version for Go 1.22.
  • After using operator-sdk 1.28 - all builds and tests worked fine.
  • One more reason for operator-sdk 1.28 -- marin3r using it.
    I'm rechecking now GO1.22 without change of Operator SDK 1.2.0 - this is PR THREESCALE-11281 - bump to go 1.22 #1045 . Will update after testing. Thank you Michal !

@valerymo
Copy link
Contributor Author

valerymo commented Dec 17, 2024

This PR will be closed, it's replaced by new one #1045 - that does not change OperatorSDK.

  • We have verified that 3scale-operator with the existing version of OperatorSDK 1.2 works with GO 1.22.
    • the reason of fails in several tests (segmentation faults) was Not old OperatorSDK 1.2, but controller-gen (see next item)
  • controller-gen update was done same way as here, from v0.9.2 to v0.14.0 due to incompatibility of controller-gen v0.9.2 with GO 1.22 (segmentation fault).
  • The SDK upgrade will be addressed in a future task. cc @MStokluska @carlkyrillos

@valerymo valerymo closed this Dec 17, 2024
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