Skip to content

feat: Add additionalCategories field to Nutanix machine details patch #525

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2024
Merged

feat: Add additionalCategories field to Nutanix machine details patch #525

merged 1 commit into from
Apr 19, 2024

Conversation

dlipovetsky
Copy link
Contributor

What problem does this PR solve?:
Adds the additionalCategories field to the Nutanix machine details patch.

Which issue(s) this PR fixes:
Fixes https://jira.nutanix.com/browse/D2IQ-100464

How Has This Been Tested?:

I amended the machine details patch test.

Special notes for your reviewer:

jimmidyson
jimmidyson approved these changes Apr 18, 2024
View reviewed changes
Copy link
Member

@jimmidyson jimmidyson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's get this merged, although I'm not totally sold on the ability to specify the same key multiple times (key in my mind is a unique identifier). However, I can't think of a different solution, other than adding value as an array, which would make the (possibly more common) use case of single value per key to be less user friendly.

@dkoshkin
Copy link
Contributor

I just tried creating a duplicate key and thats not allowed:

image
Although you can have multiple values for a key:
image

But, from what I can see the CAPX controller will actually just take whatever the last value for any duplicate keys,
https://github.com/nutanix-cloud-native/cluster-api-provider-nutanix/blob/f6e079c5dd386256a27974338a111da3b628fb9b/controllers/helpers.go#L602-L614

In which case a map[string]string would give us this enforcement for "free".

@dkoshkin
Copy link
Contributor

dkoshkin commented Apr 18, 2024 •
edited
Loading

Ha! It goes deeper, a VM can be created with 1 key but multiple values for that key, so I think there is a bug in CAPX in how it handles duplicate keys?

image

@deepakm-ntnx
Copy link
Contributor

deepakm-ntnx commented Apr 18, 2024 •
edited
Loading

But, from what I can see the CAPX controller will actually just take whatever the last value for any duplicate keys, https://github.com/nutanix-cloud-native/cluster-api-provider-nutanix/blob/f6e079c5dd386256a27974338a111da3b628fb9b/controllers/helpers.go#L602-L614

In which case a map[string]string would give us this enforcement for "free".

we should fix the CAPX to allow being able to specify same key with multiple values.

@deepakm-ntnx
Copy link
Contributor

But, from what I can see the CAPX controller will actually just take whatever the last value for any duplicate keys, https://github.com/nutanix-cloud-native/cluster-api-provider-nutanix/blob/f6e079c5dd386256a27974338a111da3b628fb9b/controllers/helpers.go#L602-L614
In which case a map[string]string would give us this enforcement for "free".

we should fix the CAPX to allow being able to specify same key with multiple values.

@dkoshkin per yannick, there was some issue with v3 apis hence this was done an in v4 its allowed to pass one key with 2 values but not in v3. need to debug more. cc @thunderboltsid
Please take a look and see if you come to the same conclusion
nutanix.dev/api_reference/apis/prism_v3.html#tag/vms/paths/~1vms/post
https://developers.nutanix.com/api-reference?namespace=vmm&version=v4.0.a1

@dlipovetsky dlipovetsky force-pushed the capx-additionalcategories branch from 8952bd2 to 11f32ae Compare April 19, 2024 15:57
@dlipovetsky
Copy link
Contributor Author

I rebased on main to resolve conflicts, and force-pushed

@dlipovetsky dlipovetsky force-pushed the capx-additionalcategories branch from 11f32ae to cd1ad35 Compare April 19, 2024 16:10
@dlipovetsky
Copy link
Contributor Author

I had some issues rebasing on main while preserving the fixups, so I squashed locally.

@dlipovetsky dlipovetsky merged commit c33c2d0 into nutanix-cloud-native:main Apr 19, 2024
15 checks passed
@github-actions github-actions bot mentioned this pull request Apr 19, 2024
Merged
faiq pushed a commit that referenced this pull request Apr 29, 2024
@jimmidyson @github-actions
release(main): v0.8.0 (#527)
dd6e9a0 
🤖 I have created a release *beep* *boop*
---


## 0.8.0 (2024-04-29)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: give mutators a clusterGetter function by @faiq in
https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pull/514it was unintelliga
* feat: get default sans via cluster object in patch handler for docker
by @faiq in
#519
* feat: adds nutanix SANs via patchHandler by @faiq in
#522
* feat: nutanix csi driver 3.0 by @faiq in
#531
* feat: Add additionalCategories field to Nutanix machine details patch
by @dlipovetsky in
#525
* feat: support setting Nutanix project on machines by @dkoshkin in
#535
* feat: Upgrade to CAPI v1.7.0 by @jimmidyson in
#555
* feat: CAPI v1.7.1 by @jimmidyson in
#560
* feat: Preserve user-managed fields when applying resources by
@dlipovetsky in
#556
* feat: Preserve user-managed fields when creating namespace by
@dlipovetsky in
#557
* feat: Added e2e test for capx cluster by @deepakm-ntnx in
#523
* feat: add kube-vip static Pod in a Nutanix handler by @dkoshkin in
#558
* feat: AWS CCM for Kubernetes v1.29 by @dkoshkin in
#564
### Fixes 🔧
* fix: updated the capx version used by @deepakm-ntnx in
#513
* fix: add omitempty to CCM Credentials struct by @dkoshkin in
#524
* fix: Add specific descriptions to Nutanix machine details fields by
@dlipovetsky in
#532
* refactor: setting ownership references to Nutanix CSI Helm Chart
Proxies by @dlipovetsky in
#565
### Other Changes
* build: Specify go1.22.2 as toolchain to fix govulncheck issues by
@jimmidyson in
#517
* build: Add metadata for latest v0.7.0 release by @jimmidyson in
#515
* refactor: Consistently import CAPI v1beta1 package as clusterv1 alias
by @jimmidyson in
#518
* build: Fix image tags in release manifests by @jimmidyson in
#516
* test(e2e): Use same versions of providers from module dependencies by
@jimmidyson in
#521
* build: update aws credentials on kind bootstrap cluster by @supershal
in
#507
* refactor: standardize the code for getting Helm values by @dkoshkin in
#500
* build: Use latest k8s for dev and test management cluster by
@jimmidyson in
#526
* docs: Add how to release doc by @jimmidyson in
#530
* build: adds a .envrc.local file for local development for dotenv by
@faiq in
#538
* refactor: create storage classes directly instead of using CRS by
@faiq in
#539
* refactor: Move API to caren.nutanix.com group by @jimmidyson in
#534
* build: Add Kubernetes v1.30.0 option for bootstrap and Docker provider
by @jimmidyson in
#541
* build: create .envrc.e2e file from caren e2e config by @supershal in
#540
* build: Only allow patch updates to k8s libs by @jimmidyson in
#551
* build: Generate CRD YAML by @jimmidyson in
#536
* build: Minor golangci-lint config updates for recent versions by
@jimmidyson in
#552
* build: generated CRDs yamls by @dkoshkin in
#553
* refactor: Use separate types for provider cluster configs by
@jimmidyson in
#537
* docs: Remove additionalCategories from required fields by @dlipovetsky
in
#543
* build: Upgrade tooling, notably go to v1.22.2 by @jimmidyson in
#561
* refactor: provider an entrypoint to the infra provider meta handlers
by @dkoshkin in
#554
* test(e2e): Add self-hosted e2e test by @jimmidyson in
#439
* build: Bundle k8s.io/* back in with sigs.k8s.io/* dependencies by
@jimmidyson in
#583
* build: Add envtest setup to e2e envrc by @jimmidyson in
#563

## New Contributors
* @deepakm-ntnx made their first contribution in
#513

**Full Changelog**:
v0.7.0...v0.8.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmidyson jimmidyson jimmidyson approved these changes

@deepakm-ntnx deepakm-ntnx deepakm-ntnx approved these changes

@dkoshkin dkoshkin dkoshkin approved these changes

@thunderboltsid thunderboltsid Awaiting requested review from thunderboltsid

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlipovetsky @dkoshkin @deepakm-ntnx @jimmidyson