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.

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

Enhance ownerReferences for Secrets used by CAPX #340

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Enhance ownerReferences for Secrets used by CAPX #340

merged 2 commits into from
Dec 8, 2023

Conversation

yannickstruyf3
Copy link
Contributor

@yannickstruyf3 yannickstruyf3 commented Dec 7, 2023 •
edited
Loading

What this PR does / why we need it:

  • Remove limitation of only allowing one ownerRef for the credential secret. In some cases multiple ownerReferences are added to the Secrets. This PR prevents CAPX from throwing an error.
    For example:
metadata:
  ownerReferences:
  - apiVersion: cluster.x-k8s.io/v1beta1
    kind: Cluster
    name: <cluster name>
    uid: <uid>
  - apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
    kind: NutanixCluster
    name: <cluster name>
    uid: <uid>
  • Adds a check to make sure a secret is only owned by one single nutanixCluster object.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
N/A

How Has This Been Tested?:
make unit-test

Special notes for your reviewer:
N/A

Release note:

- Allow multiple OwnerReferences to be set on the credentialRef secret

@yannickstruyf3 yannickstruyf3 added the enhancement New feature or request label Dec 7, 2023
@yannickstruyf3 yannickstruyf3 force-pushed the bug/allow-multiple-ownerrefs-secret branch from 532c483 to ca05ccb Compare December 7, 2023 14:19
@codecov Codecov
Copy link

codecov bot commented Dec 7, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a4044ad) 8.89% compared to head (678afbe) 10.99%.

Additional details and impacted files 
@@            Coverage Diff            @@
##            main     #340      +/-   ##
=========================================
+ Coverage   8.89%   10.99%   +2.10%     
=========================================
  Files          4        4              
  Lines       1046     1046              
=========================================
+ Hits          93      115      +22     
+ Misses       953      931      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tuxtof
tuxtof previously approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@tuxtof tuxtof left a comment

Choose a reason for hiding this comment

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

LGTM

@tuxtof tuxtof requested a review from jimmidyson December 7, 2023 14:24
jimmidyson
jimmidyson reviewed Dec 7, 2023
View reviewed changes
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
adiantum
adiantum reviewed Dec 7, 2023
View reviewed changes
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/nutanixcluster_controller_test.go Outdated Show resolved Hide resolved
@adiantum
Copy link
Contributor

adiantum commented Dec 7, 2023

/test all

@yannickstruyf3 yannickstruyf3 force-pushed the bug/allow-multiple-ownerrefs-secret branch from ca05ccb to 1c67b18 Compare December 7, 2023 19:23
@yannickstruyf3 yannickstruyf3 force-pushed the bug/allow-multiple-ownerrefs-secret branch from 1c67b18 to d0096cf Compare December 7, 2023 19:26
@thunderboltsid
Copy link
Contributor

/retest

@thunderboltsid
Copy link
Contributor

/lgtm

@thunderboltsid
Copy link
Contributor

/approve

@nutanix-cn-prow-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thunderboltsid, yannickstruyf3

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:
  • OWNERS [thunderboltsid,yannickstruyf3]

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

tuxtof
tuxtof approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@tuxtof tuxtof left a comment

Choose a reason for hiding this comment

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

LGTM

@tuxtof tuxtof merged commit f047895 into nutanix-cloud-native:main Dec 8, 2023
6 of 8 checks passed
adiantum
adiantum reviewed Dec 8, 2023
View reviewed changes
api/v1beta1/nutanixcluster_types.go
@@ -24,6 +24,9 @@ import (
)

const (
// NutanixClusterKind represents the Kind of NutanixCluster
NutanixClusterKind = "NutanixClusterKind"
Copy link
Contributor

Choose a reason for hiding this comment

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

In CAPI ClusterKind is simply "Cluster":
https://github.com/kubernetes-sigs/cluster-api/blob/9d36ddcbf8381574a86cebeacdcdfc85bafa71e9/api/v1beta1/cluster_types.go#L39

Do we really need "NutanixClusterKind" but not "NutanixCluster"?

Copy link
Member

Choose a reason for hiding this comment

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

Good spot @adiantum! These should definitely not have the Kind suffix in the string values.

api/v1beta1/nutanixmachine_types.go
@@ -28,6 +28,9 @@ import (
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

const (
// NutanixMachineKind represents the Kind of NutanixMachine
NutanixMachineKind = "NutanixMachineKind"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need "NutanixMachineKind" instead of "NutanixMachine"?

api/v1beta1/nutanixmachinetemplate_types.go
@@ -23,6 +23,11 @@ import (

// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

const (
// NutanixMachineTemplateKind represents the Kind of NutanixMachineTemplate
NutanixMachineTemplateKind = "NutanixMachineTemplateKind"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: do we really need "Kind" suffix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adiantum adiantum adiantum left review comments

@thunderboltsid thunderboltsid thunderboltsid approved these changes

@tuxtof tuxtof tuxtof approved these changes

@deepakm-ntnx deepakm-ntnx Awaiting requested review from deepakm-ntnx

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@thunderboltsid thunderboltsid

Labels
approved enhancement New feature or request lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yannickstruyf3 @adiantum @thunderboltsid @tuxtof @jimmidyson