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 shipwright-io/build to v0.13.0 #216

Merged

Conversation

adambkaplan
Copy link
Member

@adambkaplan adambkaplan commented Jun 6, 2024

Changes

Upgrade shipwright-io/build to v0.13.0, ahead of upgrading the samples and operand deployments.

This includes respective transitive updates to k8s libraries and their dependencies. As part of the upgrade, a previously identified flaky/brittle test started failing permanently. These have been skipped for the interim.

These updates also exposed a bug in the controller code, where the controller would not re-queue when a status update failed. This has now been fixed.

/kind cleanup

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Upgrade shipwright-io/build code dependencies to v0.13.0 and k8s.io to v0.27.11. These upgrades exposed a bug in the controller code, where failure to update the ShipwrightBuild object status would not trigger a requeue. This was fixed so status update failures cause the controller to re-reconcile.

@openshift-ci openshift-ci bot added release-note kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 6, 2024
@openshift-ci openshift-ci bot requested review from jkhelil and qu1queee June 6, 2024 18:20
@adambkaplan
Copy link
Member Author

/assign @ayushsatyam146

/cc @apoorvajagtap

Copy link
Member

@apoorvajagtap apoorvajagtap left a comment

Choose a reason for hiding this comment

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

@adambkaplan I believe there's a typo in k8s.io version mentioned in the release note, it should be v0.27.11 instead of v1.27.11, right?

Also, the unit test failures report
unable to retrieve the complete list of server APIs: shipwright.io/v1alpha1
As we now have set the storage version to v1beta1 with build v0.13.0, we need to update the buildStrategy samples here similar to build/samples/v1beta1.

Update the Shipwright APIs to use v0.13.0 as their baseline. This
pulled in the following dependency updates:

- k8s.io/* to v0.27.11. This corresponds to Kubernets 1.27.11 code.
- sigs.k8s.io/controller-runtime to v0.15.3

Signed-off-by: Adam Kaplan <[email protected]>
Upgrading to k8s 1.27 required a similar update to the version
Envtest simulates. While testing this locally, I discovered that the
latest branch of the setup-envtest tool requires golang 1.22. Using
the older `release-0.17` branch bypasses this issue for now.

After the update, some of the tests in
`TestShipwrightBuildReconciler_Reconcile` fail permanently. These
were previously marked as "brittle" because they used separate k8s test
clientsets. These tests are now skipped, resulting in loss of test
coverage related to operand image overrides. A refactor is in order to
ensure we only use the controller-runtime clientsets. This should come
after the upgrade to operator-sdk, which will require a restructure of
the project.

The update also exposed a latent bug in our controller logic, related
to updating the ShipwrightBuild object status. We previously were not
requeing on error, which in production can happen for a wide variety
of reasons. This bug was in fact caught by a lint check that was
skipped; this bypass has been removed to ensure we don't regress in
the future.
@adambkaplan
Copy link
Member Author

/kind bug

😌 these updates actually exposed a bug in the controller code! I observed a few things that led me to think something was off in the tests:

  1. We were still deploying Shipwright v0.12 operands, so the switch from v1alpha1 to v1beta1 storage should not be an issue.
  2. The test was failing because of an API discovery error during the setup phase - the code to check if the build strategies API existed was returning early. I suspected this was happening because we had API skew between EnvTest and our k8s.io client code. Changing EnvTest to use k8s 1.27 seems to have fixed this.
  3. I separately saw in the test logs that we were getting a "conflict" error from EnvTest. This isn't totally weird for a controller - I've seen it plenty of times in other controller logs, especially if the controller is not using server-side apply. What was weird was that there was no follow-up re-queue in the test logs. This led me to a very important 1-line bug fix.

Release note has been updated to include this bug fix.

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 7, 2024
@apoorvajagtap
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2024
@adambkaplan
Copy link
Member Author

/approve

Self-approving. TODO - may need to review DevStats to add additional approvers.

Copy link
Contributor

openshift-ci bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit e27cffb into shipwright-io:main Jun 7, 2024
4 checks passed
@adambkaplan adambkaplan deleted the bump-build-tekton branch June 7, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants