-
Notifications
You must be signed in to change notification settings - Fork 1.4k
✨ feat: add experimental clusterctl migrate command
#12843
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @ramessesii2! |
|
Hi @ramessesii2. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
db40551 to
1bf4039
Compare
clusterctl alpha migrate commandclusterctl migrate command
6f90e35 to
c2050f4
Compare
clusterctl migrate commandclusterctl migrate command
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.
did a quick first pass on migrate.go
please keep the commits squashed to 1 on amends.
| "os" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" |
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.
add new line between these two.
k8s vs non-k8s sources.
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.
Apparently, linter is not happy with the suggested change.
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.
Apparently, linter is not happy with the suggested change.
that's odd. linters normally allow you to define as many 'groups' of imports as you like.
c2050f4 to
f67227b
Compare
|
/ok-to-test |
- Experimental migration support focuses on v1beta1 to v1beta2
conversions for core Cluster API resources
Signed-off-by: Satyam Bhardwaj <[email protected]>
f67227b to
dd58aab
Compare
|
/area clusterctl |
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.
thanks for the updates. i did a bigger pass on the code today.
this is in relatively good shape, but i made some suggestions to re-organize it a bit and make api types opaque to the migrate package.
| var migrateOpts = &migrateOptions{} | ||
|
|
||
| var supportedTargetVersions = []string{ | ||
| clusterv1.GroupVersion.Version, |
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.
isn't there a programmatic way to enumerate these instead of requiring the OWNERS to manually add a new version here?
e.g. core k8s types manage that in the internal types for a given group, but in CAPI the layout is different.
|
|
||
| func runMigrate(args []string) error { | ||
| if !isSupportedTargetVersion(migrateOpts.toVersion) { | ||
| return errors.Errorf("invalid --to-version value %q: supported versions are %s", migrateOpts.toVersion, strings.Join(supportedTargetVersions, ", ")) |
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.
| return errors.Errorf("invalid --to-version value %q: supported versions are %s", migrateOpts.toVersion, strings.Join(supportedTargetVersions, ", ")) | |
| return errors.Errorf("invalid --to-version value %q. Supported versions are: %s", migrateOpts.toVersion, strings.Join(supportedTargetVersions, ", ")) |
|
|
||
| func init() { | ||
| migrateCmd.Flags().StringVarP(&migrateOpts.output, "output", "o", "", "Output file path (default: stdout)") | ||
| migrateCmd.Flags().StringVar(&migrateOpts.toVersion, "to-version", clusterv1.GroupVersion.Version, fmt.Sprintf("Target API version for migration (supported: %s)", strings.Join(supportedTargetVersions, ", "))) |
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.
| migrateCmd.Flags().StringVar(&migrateOpts.toVersion, "to-version", clusterv1.GroupVersion.Version, fmt.Sprintf("Target API version for migration (supported: %s)", strings.Join(supportedTargetVersions, ", "))) | |
| migrateCmd.Flags().StringVar(&migrateOpts.toVersion, "to-version", clusterv1.GroupVersion.Version, fmt.Sprintf("Target API version for migration. Supported versions are: %s)", strings.Join(supportedTargetVersions, ", "))) |
| parser := migrate.NewYAMLParser(scheme.Scheme) | ||
|
|
||
| targetGV := schema.GroupVersion{ | ||
| Group: clusterv1.GroupVersion.Group, |
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.
this is ok, but it hardcodes the group to the group of an imported version package.
i think a local constant that copies the group string might be better.
...or a proper mapping that takes the user version input and determines the group from it.
| "os" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" |
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.
Apparently, linter is not happy with the suggested change.
that's odd. linters normally allow you to define as many 'groups' of imports as you like.
| yamlserializer "k8s.io/apimachinery/pkg/runtime/serializer/yaml" | ||
| yamlutil "k8s.io/apimachinery/pkg/util/yaml" | ||
|
|
||
| clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" |
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.
ditto. the GVKs must be opaque to the internal/migrate package i would say. it can accept a list of known source and target GVKs.
|
|
||
| const ( | ||
| // ResourceTypeCoreV1Beta1 identifies v1beta1 core ClusterAPI resources. | ||
| ResourceTypeCoreV1Beta1 ResourceType = iota |
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.
the classification of input GVKs can be done based on provided GVK inputs as params.
the first const in the enum here could be a generic ResourceTypeSupported (or similar for a type that can be parsed on migrated).
|
|
||
| func (p *YAMLParser) classifyResource(gvk schema.GroupVersionKind) ResourceType { | ||
| if p.isCoreCAPIGroup(gvk.Group) { | ||
| if gvk.Version == "v1beta1" { |
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.
best to check from the input GVK i mention above instead of hardcoding known versions in string literals.
| return ok | ||
| } | ||
|
|
||
| var coreCapiGroups = map[string]struct{}{ |
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.
put vars on top of the doc.
|
|
||
| Examples: | ||
| # Migrate from file to stdout | ||
| clusterctl migrate cluster.yaml |
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.
would migrate from new to old be supported?
e.g. v1beta2 -> v1beta1
Implements a new
clusterctl migratecommand to convert core CAPI resources i.e.,cluster.x-k8s.ioresources between API versions, currently supporting v1beta1 → v1beta2 migration.✅ Tests
✅ Docs
Implements #12716