Skip to content

Conversation

heliapb
Copy link
Member

@heliapb heliapb commented Feb 7, 2025

Closes #26

@heliapb heliapb changed the title feat - etcd feat - object etcd migration Feb 7, 2025
@heliapb heliapb marked this pull request as ready for review February 10, 2025 23:43
@heliapb heliapb requested a review from a team as a code owner February 10, 2025 23:43
@heliapb heliapb force-pushed the feat/etcd branch 2 times, most recently from fc2bda0 to 1c322be Compare February 11, 2025 22:10
Signed-off-by: Hélia Barroso <[email protected]>
@nicolastakashi
Copy link
Collaborator

@simonpasquier can you help review this? I'm not sure about this change.

Copy link
Collaborator

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I had a look at the cert-manager CLI and they have a similar functionality. I wonder if we couldn't import/copy their logic?

https://github.com/cert-manager/cmctl/blob/main/pkg/upgrade/migrateapiversion/migrator.go

)

func MigrateCRDs(ctx context.Context, clientSets *k8sutil.ClientSets) error {
crds, err := clientSets.APIExtensionsClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we hardocde the list of CRDs supported by the tool? E.g. for now we only have alertmanagerconfigs (+ scrapeconfigs eventually).

return fmt.Errorf("failed to list CRDs: %w", err)
}

for _, crd := range crds.Items {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) To simplify the reading and error handling, I would loop over the CRDs to build the list of (CRD + desired version) and then migrate the result in another loop.

for _, namespace := range namespaces.Items {
ns := namespace.Name

customResourcesInstances, err := clientSets.DClient.Resource(crdResourceVersion).Namespace(ns).List(ctx, metav1.ListOptions{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you pass an empty namespace, I suppose that it will return resources for all namespace (not tested though).

Suggested change
customResourcesInstances, err := clientSets.DClient.Resource(crdResourceVersion).Namespace(ns).List(ctx, metav1.ListOptions{})
customResourcesInstances, err := clientSets.DClient.Resource(crdResourceVersion).Namespace("").List(ctx, metav1.ListOptions{})

@simonpasquier
Copy link
Collaborator

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.

Object etcd migration
3 participants