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

updating dependencies, operator-sdk versions to release operator #141

Merged

Conversation

acornett21
Copy link
Contributor

@acornett21 acornett21 commented Jan 17, 2023

OCO wasn't working on clusters where Pipelines Operator got updated to 1.8.3. When looking into this more, different versions of OpenShift had different versions of Pipelines Operator. This PR aims to address that by switching to a newest dependency model, where OLM will apply the newest Pipeliens Operator that it has in a cluster. The pipeline also addresses

  • Updating/Migrating Operator SDK to 1.26.0.
  • Updating Go Dependencies.
    • Updating Go to 1.19 to satisfy dependency changes.
  • A few files got formatted due to these dependency changes.

Testing:

OpenShift Version Operator Pipeline Version Install Successful
4.8 1.5.2 Passed
4.9 1.7.3 Passed
4.10 1.8.3 Passed
4.11 1.8.3 Not Tested
4.12 1.8.3 Passed

Note: 4.11 was not tested since the previous version was certified here, and has the same dependencies as 4.10 and 4.12

Signed-off-by: Adam D. Cornett adc@redhat.com

@acornett21 acornett21 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2023
@openshift-ci openshift-ci bot requested a review from skattoju January 17, 2023 22:13
@acornett21 acornett21 force-pushed the update_dependencies branch 3 times, most recently from d21edc6 to eaee145 Compare January 17, 2023 22:36
@acornett21 acornett21 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2023
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

A few questions and a change.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

More comments.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@acornett21 acornett21 force-pushed the update_dependencies branch 4 times, most recently from 1111689 to 739dc84 Compare January 20, 2023 17:41
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023
@@ -2,4 +2,4 @@ dependencies:
- type: olm.package
value:
packageName: openshift-pipelines-operator-rh
version: "1.7.1"
version: ">=1.5.2"
Copy link

Choose a reason for hiding this comment

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

We're supporting older versions of openshift-pipelines-operator-rh with this directive, right? Is that the goal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great question. Since we have a hard dependency on the pipelines operator, we need to may sure any version greater then 1.5.2 is in the cluster (since this is the version that has the CRD's we rely on). Ideally, as pipelines operator released new versions we'd update to the latest version, but they do not release all new versions to all OpenShift versions. So to make sure that OCO (this operator) works on all clusters we need to support 4.8 onward, this syntax makes sure we get the newest version in whatever cluster runs this operator. Hope this is clear.

Copy link

Choose a reason for hiding this comment

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

Got it, and the common alternative to this approach is to release multiple streams, each with a hard-pinned dependency vs. this which is more flexible but releases a single stream of this project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'm not sure how we'd go to a multi-stream approach with a hard pinned version at this point(in a logistical sense), nor do I think we want that kind of overhead when trying to certify.

Copy link

@komish komish left a comment

Choose a reason for hiding this comment

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

Left a few comments, which are mostly nits. As we discussed offline, I don't really consider myself a maintainer of this project, and so I don't know much of its operation. That said, logistically, this patch seems fine.

To that end.

/lgtm

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
…OpenShift 4.12

Signed-off-by: Adam D. Cornett <adc@redhat.com>
Copy link
Contributor

@bcrochet bcrochet left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@acornett21 acornett21 merged commit ece5033 into redhat-openshift-ecosystem:main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants