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

small fix on test_and_publish_OLM_bundle #1603

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

DanielePalaia
Copy link
Contributor

@DanielePalaia DanielePalaia commented Apr 4, 2024

Small fix on test_and_publish_OLM_bundle action workflow.

There is a small issue during the last release:
https://github.com/rabbitmq/cluster-operator/actions/runs/8539313818/job/23393756084

Basically the sh shell doesn't recognize properly the line:
BUNDLE_VERSION=${BUNDLE_VERSION:1}

giving a bad substitution error.

This should fix this issue.

This PR is also bumping GO to 1.22 and golang.org/x/net to v0.23.0 in order to avoid the new vulnerability issue detected in https://github.com/rabbitmq/cluster-operator/actions/runs/8552381205/job/23433359006

It is also adding some missing crd fields to our CRD after a make manifests.
It maybe related to be674d6

@DanielePalaia DanielePalaia force-pushed the olm_action_small_fix branch from 3e53fb6 to 3e7ab4f Compare April 4, 2024 09:39
@DanielePalaia DanielePalaia changed the title small fix on test_and_publish_OLM_bundle small fix on test_and_publish_OLM_bundle / bumping GO and x/net / updating CRD Apr 4, 2024
@DanielePalaia DanielePalaia requested a review from mkuratczyk April 4, 2024 09:53
Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I don't oppose explicitly to these changes, but a few observations:

Bumping to Go 1.22 may break some people. We found a user in the past who was consuming this repo as a Go library. The same applies for the Topology Operator, it may break it, as Go is becoming more strict with the go directive in go.mod, so that you don't consume modules from higher library versions. The fix seems possible in Go 1.21, so I don't think it's necessary to bump to 1.22.

There are 3 arguably related changes in this PR. Whilst all the changes are valid, I think it's better to submit separate PRs for each of them. Not only it helps with understanding the reason behind each change, it also makes it easier to revert a specific change, shall the need arise. As it stands, reverting this PR would revert 3 changes, whereas we may only want to revert one (in the hypothetical scenario that we wanted to revert)

@DanielePalaia
Copy link
Contributor Author

@Zerpet Yes I agree with your point. I was thinking the same. I will submit 3 PRs then and I will keep this one for the original fix related to OLM. For what concerns the vulnerability check I originally tried to bump just golang.org/x/net to v0.23.0 and not golang but apparently it wasn't enough. I will try to do other tests on the new Prs

@DanielePalaia DanielePalaia force-pushed the olm_action_small_fix branch from 3e7ab4f to 463d3d2 Compare April 4, 2024 12:59
@DanielePalaia DanielePalaia changed the title small fix on test_and_publish_OLM_bundle / bumping GO and x/net / updating CRD small fix on test_and_publish_OLM_bundle Apr 4, 2024
@Zerpet Zerpet mentioned this pull request Apr 4, 2024
@DanielePalaia DanielePalaia force-pushed the olm_action_small_fix branch from 463d3d2 to 2ba5d92 Compare April 4, 2024 15:02
@DanielePalaia DanielePalaia merged commit 0c7cfd2 into main Apr 4, 2024
15 checks passed
@DanielePalaia DanielePalaia deleted the olm_action_small_fix branch April 4, 2024 15:27
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