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.

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

ocm: add klusterlet addon config and unit tests #478

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

klaskosk
Copy link
Contributor

@klaskosk klaskosk commented Jun 14, 2024

Adds the KlusterletAddonConfig to the existing ocm package along with unit tests for the new resource.

pkg/kac/kac.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@kononovn kononovn left a comment

Choose a reason for hiding this comment

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

Looks good just few minor comments.

pkg/kac/kac.go Outdated Show resolved Hide resolved
pkg/kac/kac.go Outdated Show resolved Hide resolved
pkg/kac/kac.go Outdated Show resolved Hide resolved
@klaskosk
Copy link
Contributor Author

@kononovn Thanks for your review, I've updated the PR accordingly

pkg/kac/kac.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@kononovn kononovn left a comment

Choose a reason for hiding this comment

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

1 last comment to fix

kononovn
kononovn previously approved these changes Jun 18, 2024
Copy link
Collaborator

@kononovn kononovn left a comment

Choose a reason for hiding this comment

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

/lgtm

@trewest
Copy link
Collaborator

trewest commented Jun 18, 2024

@klaskosk Shouldn't this be in the ocm pkg?

@klaskosk
Copy link
Contributor Author

@trewest I'm not too familiar with what counts as OCM or not, but if you think it would be better as part of OCM I can move it

@trewest
Copy link
Collaborator

trewest commented Jun 18, 2024

@trewest I'm not too familiar with what counts as OCM or not, but if you think it would be better as part of OCM I can move it

@klaskosk yeah I typically try to group resources by group as much as I can

$ oc explain klusterletaddonconfigs.agent.open-cluster-management.io | head -n 3
GROUP:      agent.open-cluster-management.io
KIND:       KlusterletAddonConfig
VERSION:    v1
$ oc explain managedcluster | head -n 3
GROUP:      cluster.open-cluster-management.io
KIND:       ManagedCluster
VERSION:    v1

If it isn't too much trouble, let's move it into the ocm pkg

@klaskosk klaskosk force-pushed the kac-package branch 2 times, most recently from 449244b to c3b8c36 Compare June 18, 2024 15:35
@klaskosk klaskosk changed the title kac: add new kac package and unit tests ocm: add klusterlet addon config and unit tests Jun 18, 2024
@klaskosk
Copy link
Contributor Author

@trewest Ah, thanks for the explanation. I've updated the PR to move it to the ocm package

pkg/ocm/kac.go Outdated
// Object of the KlusterletAddonConfig as it is on the cluster.
Object *kacv1.KlusterletAddonConfig
// apiClient used to interact with the cluster.
apiClient *clients.Settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be runtimeclient.Client

Copy link
Collaborator

@trewest trewest left a comment

Choose a reason for hiding this comment

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

Looks good, just need to limit the apiClient

Adds the KlusterletAddonConfig to the existing ocm package along with unit tests for the new resource.
Copy link
Collaborator

@trewest trewest left a comment

Choose a reason for hiding this comment

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

/lgtm

@trewest trewest merged commit 8a6c53c into openshift-kni:main Jun 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants