Skip to content

Conversation

@patrickdillon
Copy link
Contributor

The verify-capi-manifests script was failing on nutanix, which referenced an individual commit--not a release. Fixed that by adding git fetch and scripting some dependency management.

Also included fixup of mismatched CRDs.

Nutanix was crashing the script due to:
- using an unavailable git ref (fixed by adding git fetch)
- dependency issues: kustomize & controller-gen not found
These are the correct versions of the CRDs, according to the
verify-capi-manifests script.
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 28, 2025
@openshift-ci-robot
Copy link
Contributor

@patrickdillon: This pull request explicitly references no jira issue.

Details

In response to this:

The verify-capi-manifests script was failing on nutanix, which referenced an individual commit--not a release. Fixed that by adding git fetch and scripting some dependency management.

Also included fixup of mismatched CRDs.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from barbacbd and rwsu May 28, 2025 19:16
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

/approve

Looks good! I just have a small nit and a question.

Comment on lines 65 to 67


# Install `controller-gen` & `kustomize`, which are needed by nutanix, if not present
Copy link
Member

Choose a reason for hiding this comment

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

I thought, usually, these tools are version-pinned and downloaded automatically on their side. Let me see if I can contribute upstream.

Also, took this chance to nit pick an extra newline haha

Suggested change
# Install `controller-gen` & `kustomize`, which are needed by nutanix, if not present
# Install `controller-gen` & `kustomize`, which are needed by nutanix, if not present

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hopefully, someone will take a look soon haha 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this would definitely be better--thanks for doing it right. I tried to ping some upstream reviewers on your PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @patrickdillon ! From comments upstream, they "did" similarly but moved to dev-box. So it's not great to make them switch again so I closed it 😓

So, currently this way is best we can do :D I am only concerned as it is using "latest"...

clone_path="$(mktemp -d)"
git clone "${repo_origin}" "${clone_path}"
pushd "${clone_path}"
git fetch "${repo_origin}" "${revision}"
Copy link
Member

Choose a reason for hiding this comment

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

One of the concerns I observed: there are small diffs in infra manifest between the released assets and the generated ones in the same commit.

One example: #9704 (review). Or similarly, if I run the ./hack/verify-capi-manifests.sh again in this PR here (released v1.17.5 azure-infra manifest). I see:

diff --git a/data/data/cluster-api/azure-infrastructure-components.yaml b/data/data/cluster-api/azure-infrastructure-components.yaml
index 215de54e78..abca06bac3 100644
--- a/data/data/cluster-api/azure-infrastructure-components.yaml
+++ b/data/data/cluster-api/azure-infrastructure-components.yaml
@@ -80694,7 +80694,7 @@ stringData:
   AZURE_SUBSCRIPTION_ID: ""
   AZURE_SYNC_PERIOD: ${AZURE_SYNC_PERIOD:=""}
   AZURE_TENANT_ID: ""
-  AZURE_USER_AGENT_SUFFIX: cluster-api-provider-azure/v1.17.5
+  AZURE_USER_AGENT_SUFFIX: cluster-api-provider-azure/main
 type: Opaque
 ---
 apiVersion: v1
@@ -80980,8 +80980,8 @@ spec:
           valueFrom:
             fieldRef:
               fieldPath: metadata.namespace
-        image: registry.k8s.io/cluster-api-azure/cluster-api-azure-controller:v1.17.5
-        imagePullPolicy: IfNotPresent
+        image: gcr.io/k8s-staging-cluster-api-azure/cluster-api-azure-controller:main
+        imagePullPolicy: Always
         livenessProbe:
           httpGet:
             path: /healthz

I guess it does not matter much. But one idea is, for this script, to first find the released assets. If any, use that. If not, generate from code (maybe future improvement). WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@patrickdillon I have a sketch of the idea above here: patrickdillon#2 (i.e. against this PR branch) if you'd like to have a quick look 😃

One of the benefits I can see is the shorter time to run the script (i.e. time ./hack/verify-capi-manifests.sh):
Before: 4m12.324s
After: 2m29.181s (with patrickdillon#2)

Copy link
Member

Choose a reason for hiding this comment

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

Updated: #9752 (comment) is added before CAPZ PR was merged. Though, I guess the question for checking released assets first is still valid?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tthvo

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

The pull request process is described here

Details 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 May 28, 2025
tthvo added 2 commits May 28, 2025 23:22
This improvement includes trying out the released assets first if any.
In case that fails (i.e. not a version, or non-existing asset URL), fall
back to generate from pinned revision.
@barbacbd
Copy link
Contributor

barbacbd commented Jun 2, 2025

/hold

Adding a hold as a safety precaution. The upstream PR should merge and a new CRD should be regenerated either off of master or when a release puts one out (where the PR is included 😄 )

In regards to

Updated: https://github.com/openshift/installer/pull/9752#discussion_r2112903348
is added before CAPZ PR was merged. Though, I guess the question for checking
released assets first is still valid?

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2025
@patrickdillon
Copy link
Contributor Author

patrickdillon commented Jul 30, 2025

The current version of this pr has been breaking because the nutanix gitref is not actually merged (it is pointing to a pr). #9855 should unblock this.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2025
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Details

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-sigs/prow repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 11, 2025

@patrickdillon: The following tests 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/e2e-vsphere-externallb-ovn d4b8b4d link false /test e2e-vsphere-externallb-ovn
ci/prow/okd-scos-e2e-aws-ovn d4b8b4d link false /test okd-scos-e2e-aws-ovn
ci/prow/shellcheck d4b8b4d link true /test shellcheck
ci/prow/yaml-lint d4b8b4d link true /test yaml-lint

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 11, 2025
@tthvo
Copy link
Member

tthvo commented Dec 11, 2025

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 11, 2025
@patrickdillon
Copy link
Contributor Author

/close

in favor of #10212

@openshift-ci openshift-ci bot closed this Jan 15, 2026
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 15, 2026

@patrickdillon: Closed this PR.

Details

In response to this:

/close

in favor of #10212

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-sigs/prow repository.

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants