externalize migration commands #20927
Conversation
openshift-ci-robot
added
the
size/L
|
/assign soltysh enj |
| @@ -75,17 +73,14 @@ var ( | |||
| type MigrateImageReferenceOptions struct { | |||
| migrate.ResourceOptions | |||
|
|
|||
| Client imagetypedclient.ImageStreamsGetter | |||
| Client imagev1typedclient.ImageStreamsGetter | |||
| Mappings ImageReferenceMappings | |||
| UpdatePodSpecFn func(obj runtime.Object, fn func(*corev1.PodSpec) error) (bool, error) | |||
There was a problem hiding this comment.
polymorphichelpers.UpdatePodSpecForObjectFunc
pkg/oc/cli/admin/migrate/migrator.go
Outdated
| obj, err := legacyscheme.Scheme.ConvertToVersion(info.Object, info.Mapping.GroupVersionKind.GroupVersion()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // TODO: PrintObj is not correct for YAML - it should inject document separators itself |
There was a problem hiding this comment.
Is it all still needed, isn't our new printing stack capable of handling these?
There was a problem hiding this comment.
The new printing stack only handles api lists correctly when printing yaml or json. The yaml printer cannot know to print --- after every yaml object that it prints without keeping state or just printing --- every time its PrintObj method is called.
I am not a fan of having a yaml printer that knows about the last object that was printed, and I think expecting consumers of the printer to pass an api list is reasonable.
|
/approve |
openshift-ci-robot
added
the
approved
juanvallejo
force-pushed
the
jvallejo/externalize-migrate-cmds
branch
from
42c1d5c to
684272e
Compare
|
@soltysh thanks for the review, comments addressed |
| @@ -135,6 +138,38 @@ type ResourceOptions struct { | |||
| genericclioptions.IOStreams | |||
| } | |||
|
|
|||
| func NewResourceOptions(streams genericclioptions.IOStreams) *ResourceOptions { | |||
| return &ResourceOptions{ | |||
| PrintFlags: genericclioptions.NewPrintFlags("migrated").WithTypeSetter(scheme.Scheme), | |||
There was a problem hiding this comment.
New printing stack. We have been pushing the use of a single code-path (this one) to print anything in the cli. The genericclioptions package was recently moved to its own repo upstream (k8s.io/cli-runtime) which means that oc will become a primary consumer of that package.
The main gist behind the new printing stack is that it requires all consumers to pass it external objects only. It also enforces additional metadata for each object that it receives, so that anything that is printed to the user is displayed accurately, and in a format consumable by other oc commands. We start out with a struct that the command can use to bind all printing-related flags at once. Then from that struct, use flag values provided by the user to generate the appropriate printer.
The type-setter is temporary, but for now it allows us to attempt to retrieve the gvk for runtime.Objects that may not have it set.
|
|
||
| usage := "Filename, directory, or URL to docker-compose.yml file to use" | ||
| kcmdutil.AddJsonFilenameFlag(c.Flags(), &o.Filenames, usage) | ||
| } | ||
|
|
||
| func (o *ResourceOptions) Complete(f kcmdutil.Factory, c *cobra.Command) error { | ||
| o.Output = kcmdutil.GetFlagString(c, "output") | ||
|
|
||
| if o.DryRun { | ||
| o.PrintFlags.Complete("%s (dry run)") |
There was a problem hiding this comment.
This looks weird to me as well, but it is the current solution that we have in place for generating dry-run messages.
By default, if no --output argument is supplied by a user, the printing stack's ToPrinter() method returns a success-message printer, which as the name implies, prints a successful message (such as pod/foo-bar successfully deleted or something similar). In the case of a dry-run for the command, the only way to currently note that in a successful output (using the printing stack) is to go through the flags' Complete method.
| @@ -310,7 +345,7 @@ func (o *ResourceOptions) Complete(f kcmdutil.Factory, c *cobra.Command) error { | |||
| if o.Unstructured { | |||
| o.Builder.Unstructured() | |||
| } else { | |||
| o.Builder.WithScheme(ocscheme.ReadingInternalScheme) | |||
| o.Builder.WithScheme(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...) | |||
There was a problem hiding this comment.
Yep, this scheme contains our resources + kubectl resources. We have been using it to prioritize getting external, grouped resources across commands here and upstream
| @@ -267,13 +263,11 @@ func (o *MigrateAPIStorageOptions) Complete(f kcmdutil.Factory, c *cobra.Command | |||
| clientConfigCopy.Burst = 99999 | |||
| clientConfigCopy.QPS = 99999 | |||
|
|
|||
| client, err := dynamic.NewForConfig(clientConfigCopy) | |||
| o.client, err = dynamic.NewForConfig(clientConfigCopy) | |||
There was a problem hiding this comment.
?
We were setting the client a few lines below. Figured it was cleaner to assign it directly
There was a problem hiding this comment.
I am just not a fan of that pattern.
There was a problem hiding this comment.
@enj what's wrong with it? Why would you prefer setting a temporary variable and then assign it to a struct member instead of direct assignment? I'm curious.
There was a problem hiding this comment.
Feels weird to me to set a struct member before first checking the error. But it is really just a preference.
juanvallejo
force-pushed
the
jvallejo/externalize-migrate-cmds
branch
from
684272e to
21bf987
Compare
openshift-ci-robot
added
size/XL
|
/test gcp |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, juanvallejo, soltysh 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:
Approvers can indicate their approval by writing |
Updates migration commands to use external clients and object versions.
cc @soltysh @deads2k @enj