-
Notifications
You must be signed in to change notification settings - Fork 292
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
Changes from all commits
e8d2e9a
701178f
365db05
095b205
b99561a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yup definitely a follow-up |
||
versions: | ||
- additionalPrinterColumns: | ||
- jsonPath: .spec.hardware.cpus | ||
|
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
cluster-api-provider-vsphere/controllers/vmware/test/controllers_suite_test.go
Line 81 in b99561a