-
Notifications
You must be signed in to change notification settings - Fork 295
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
🌱 Bump vm-operator to v1.8.6 #2914
🌱 Bump vm-operator to v1.8.6 #2914
Conversation
Changes look good so far, pending linter & verify |
/cherry-pick release-1.10 |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you. In response to this:
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/test-infra repository. |
/test |
@fabriziopandini: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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/test-infra repository. |
/test pull-cluster-api-provider-vsphere-e2e-supervisor-blocking-main |
/lgtm |
LGTM label has been added. Git tree hash: d694499bae45082873a547259c38fb3744803764
|
Needs a |
/test pull-cluster-api-provider-vsphere-e2e-supervisor-blocking-main |
@@ -14,7 +14,7 @@ spec: | |||
shortNames: | |||
- vmclass | |||
singular: virtualmachineclass | |||
scope: Cluster | |||
scope: Namespaced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if that's the thing that breaks the integration tests.
IIRC Namespaced is the correct case, so we should adapt integration tests accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing we use different CRDs for e2e tests? That would be not ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
I don't have historical context on integration tests, it is on my list to look into them and if possible get rid of them now that we have proper supervisor tests
For now, I have updated the local copy of CRDs to see if I can get them back to green
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup definitely a follow-up
/test pull-cluster-api-provider-vsphere-test-integration-main |
6ad4c49
to
b99561a
Compare
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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/test-infra repository. I understand the commands that are listed here. |
/test pull-cluster-api-provider-vsphere-verify-main |
@@ -220,8 +220,14 @@ spec: | |||
type: object | |||
type: array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these files still used? (Wondering because there's integration test in the name)
Fine to follow up in a separate PR if we still use it for e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to drop them. E2E tests install these CRDs via the vm-operator's manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just on my phone. But I think at least controller unit tests are using them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely fine with moving on with this PR. Maybe a follow up would be nice.
Although integration test might be appropriate as a folder name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those CRD are still used by supervisor's controller tests using envetest
I have opened #2922 to start improving how do we manage those CRDs, but I don't think we can drop them now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, they are referenced at
filepath.Join(root, "config", "deployments", "integration-tests", "crds"), |
One question. Up to you /lgtm /hold |
LGTM label has been added. Git tree hash: 500dd5a3eeb9bfbb636655b7678bf80824e3bb00
|
/approve /hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi, sbueringer 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 |
@sbueringer: #2914 failed to apply on top of branch "release-1.10":
In response to this:
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/test-infra repository. |
/cherry-pick release-1.10 |
@fabriziopandini: new pull request created: #2925 In response to this:
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/test-infra repository. |
What this PR does / why we need it:
Also picking up latest github.com/vmware-tanzu/net-operator-api