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

Update commands with resource builders to use external versions #20400

Closed

Conversation

juanvallejo
Copy link
Contributor

Updates any remaining commands that use resource builders to deal with external versions

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2018
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 23, 2018
@@ -256,6 +254,7 @@ func (o *DebugOptions) Complete(cmd *cobra.Command, f kcmdutil.Factory, args []s

o.Builder = f.NewBuilder

// TODO: not sure what to do with this helper - new duplicate for external versions?
o.AddEnv, o.RemoveEnv, err = utilenv.ParseEnv(envArgs, nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k @soltysh still not sure what to do with this helper, duplicate it to deal with externals? - it depends on other env utils

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -562,6 +554,7 @@ func (o *DebugOptions) getContainerImageCommand(pod *kapi.Pod, container *kapi.C
image, _ = o.getContainerImageViaImageStreamImport(container)
}

// TODO: DockerImageMetadata is a RawExtension in the external api - how to access config?
if image == nil || image.DockerImageMetadata.Config == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k @soltysh Not sure how to access config here - DockerImageMetadata is a RawExtension in the external api

@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from ceb15f9 to 3aabb30 Compare July 23, 2018 22:10
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 23, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from 3aabb30 to a32fc18 Compare July 23, 2018 22:44
@juanvallejo
Copy link
Contributor Author

cc @deads2k @soltysh

@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 2 times, most recently from 9c2cad7 to 130c684 Compare July 25, 2018 14:53
@juanvallejo juanvallejo changed the title [WIP] Update commands with resource builders to use external versions Update commands with resource builders to use external versions Jul 25, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2018
// TODO: find a way to do this with the external api
// convert image to internal in order to extract its imagemetadata config
internalImage := &internalimage.Image{}
if err := oapiv1.Convert_v1_Image_To_image_Image(image, internalImage, nil); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k Is there a more elegant way to access an image's DockerImageMetadata.Config field? The external api stores the DockerImageMetadata field as a RawExtension

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll have to live with it for now.

@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from 130c684 to 3883384 Compare July 25, 2018 22:00
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 25, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 2 times, most recently from 117694b to b86760e Compare July 25, 2018 22:58
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from b86760e to e26a263 Compare July 26, 2018 18:05
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 3 times, most recently from 7a3455b to 2cee3ef Compare July 26, 2018 20:30
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 3 times, most recently from 9fe3318 to c8372d2 Compare July 31, 2018 14:08
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 3 times, most recently from 702c833 to f9df752 Compare August 1, 2018 02:12
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 2 times, most recently from 52dcaba to 0d9e138 Compare August 1, 2018 14:42
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from 0d9e138 to e802648 Compare August 1, 2018 22:52
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch 2 times, most recently from 0adbadb to 8ca712f Compare August 2, 2018 02:18
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from 8ca712f to 9095e38 Compare August 2, 2018 02:56
@juanvallejo juanvallejo force-pushed the jvallejo/more-cmds-externals branch from 9095e38 to 8a8b443 Compare August 2, 2018 03:13
@openshift-ci-robot
Copy link

@juanvallejo: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/end_to_end 8a8b443 link /test end_to_end
ci/openshift-jenkins/extended_clusterup 8a8b443 link /test extended_clusterup
ci/openshift-jenkins/cmd 8a8b443 link /test cmd
ci/prow/gcp 8a8b443 link /test gcp
ci/openshift-jenkins/extended_conformance_install 8a8b443 link /test extended_conformance_install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

if _, err := o.SecretsInterface.Create(secret); err != nil {
return err
}
// TODO: sweep codebase removing implied --dry-run behavior when -o is specified
Copy link
Contributor

Choose a reason for hiding this comment

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

@juanvallejo create an issue about it.

@soltysh
Copy link
Contributor

soltysh commented Aug 2, 2018

Closed in favor of #20514.

@soltysh soltysh closed this Aug 2, 2018
@juanvallejo juanvallejo deleted the jvallejo/more-cmds-externals branch August 2, 2018 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants