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.

Sign up for GitHub

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

Jump to bottom

UPSTREAM: 53464: use unstructured builder - oc export #16665

Merged
merged 3 commits into from
Oct 26, 2017
Merged

UPSTREAM: 53464: use unstructured builder - oc export #16665

merged 3 commits into from
Oct 26, 2017

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Oct 3, 2017 •
edited
Loading

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1496755

Use unstructured builder to obtain resources before exporting.
This allows us to support service-catalog resources.

Before

$ oc export serviceinstance/my-service-instance
the provided version "servicecatalog.k8s.io/v1alpha1" has no relevant versions: group servicecatalog.k8s.io has not been registered
no matches for servicecatalog.k8s.io/, Kind=ServiceInstance

After

$ oc export serviceinstance/my-service-instance
apiVersion: servicecatalog.k8s.io/v1alpha1
kind: ServiceInstance
metadata:
  creationTimestamp: ""
  deletionTimestamp: null
  finalizers:
  - kubernetes-incubator/service-catalog
  generation: 1
  name: my-service-instance
  namespace: ""
...

cc @openshift/cli-review

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 3, 2017
@juanvallejo
Copy link
Contributor Author

/retest

1 similar comment
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2017
@juanvallejo juanvallejo changed the title use unstructured builder - oc export [WIP] use unstructured builder - oc export Oct 4, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch from 0cc4eba to b276e58 Compare October 4, 2017 20:25
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 4, 2017
juanvallejo
juanvallejo commented Oct 4, 2017
View reviewed changes
....io/kubernetes/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured.go
@@ -392,6 +392,10 @@ func (u *Unstructured) GetCreationTimestamp() metav1.Time {

func (u *Unstructured) SetCreationTimestamp(timestamp metav1.Time) {
ts, _ := timestamp.MarshalQueryParameter()
if len(ts) == 0 {
u.setNestedField(nil, "metadata", "creationTimestamp")
Copy link
Contributor Author

@juanvallejo juanvallejo Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@liggitt this fixes the issue I had mentioned in our conversation earlier.
Exporting unstructured objects with an empty string as the value of their
creationTimestamp field was causing an error [1] when trying to re-create
those objects with oc create.

$ oc export bc/foo > foo.yaml
...
$ oc create -f foo.yaml
Error from server (BadRequest): BuildConfig in version "v1" cannot be handled as a BuildConfig: parsing time "" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "2006"

Will open upstream PR if this looks okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ironcladlou you spent a lot of time in here. This look good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Assigning a nil key to the internal map seems to be a perpetuation of the problem outlined in kubernetes/kubernetes#48211 (related to several bugs).

Potentially diverging from upstream by carrying a patch for this particular change seems riskier than usual given the history of nuanced serialization problems in this type. My suggestion would be to make an upstream commit a prerequisite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assigning a nil key to the internal map seems to be a perpetuation of the problem outlined in kubernetes/kubernetes#48211 (related to several bugs).

Potentially diverging from upstream by carrying a patch for this particular change seems riskier than usual given the history of nuanced serialization problems in this type. My suggestion would be to make an upstream commit a prerequisite.

@ironcladlou you'll be the likely reviewer upstream. Will it merge upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

@juanvallejo, can you extract this patch into an upstream PR and include a unit test demonstrating that it fixes a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ironcladlou
Upstream PR opened here: kubernetes/kubernetes#53464

@juanvallejo juanvallejo removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 4, 2017
@juanvallejo juanvallejo changed the title [WIP] use unstructured builder - oc export use unstructured builder - oc export Oct 4, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch from 693f481 to 47a724c Compare October 4, 2017 21:44
@juanvallejo juanvallejo changed the title use unstructured builder - oc export UPSTREAM: 53464: use unstructured builder - oc export Oct 4, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch from 47a724c to e158a97 Compare October 4, 2017 22:10
@juanvallejo
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 5, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch from 2f24c0a to 11d6a4c Compare October 5, 2017 19:28
@0xmichalis
Copy link
Contributor

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch 2 times, most recently from 92ae9ea to 26d5087 Compare October 6, 2017 17:50
deads2k
deads2k reviewed Oct 11, 2017
View reviewed changes
pkg/oc/cli/cmd/export.go
@@ -133,12 +142,44 @@ func RunExport(f *clientcmd.Factory, exporter Exporter, in io.Reader, out io.Wri
newInfos := []*resource.Info{}
errs := []error{}
for _, info := range infos {
converted := false

// convert unstructured object to runtime.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with the approach, but only if we deprecate oc export and stop adding any new features to it.

@fabianofranz it's a pretty ugly hammer, but it behaves slightly better on unknown types.

@juanvallejo
Copy link
Contributor Author

/retest

1 similar comment
@juanvallejo
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/export-unstructured-resources branch from 26d5087 to dd1c7af Compare October 16, 2017 21:27
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo
Copy link
Contributor Author

/test extended_conformance_gce

@juanvallejo
Copy link
Contributor Author

@deads2k @fabianofranz friendly ping

@deads2k
Copy link
Contributor

deads2k commented Oct 18, 2017

@juanvallejo you got a release note ready?

/approve

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2017
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Oct 18, 2017 •
edited
Loading

@deads2k

@juanvallejo you got a release note ready?

Modified the "export" command to be able to deal with resources not known to the client.
Resources, such as thirdparty-resources and service catalog resources, can now be used with this command.

@fabianofranz
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, fabianofranz, juanvallejo

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@juanvallejo
Copy link
Contributor Author

@fabianofranz this solves a cli blocker, wondering if we can add the "kind bug" label here as well?

@juanvallejo
Copy link
Contributor Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 25, 2017
@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install_update

@juanvallejo
Copy link
Contributor Author

/test extended_conformance_gce

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 26, 2017 •
edited
Loading

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

Test name Commit Details Rerun command
ci/openshift-jenkins/build b276e58 link /test build
ci/openshift-jenkins/origin/verify 11d6a4c link /test origin-verify
ci/openshift-jenkins/origin/unit 11d6a4c link /test origin-ut
ci/openshift-jenkins/experimental/unit 26d5087 link /test origin-ut

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 400b303 into openshift:master Oct 26, 2017
@juanvallejo juanvallejo deleted the jvallejo/export-unstructured-resources branch October 26, 2017 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@ironcladlou ironcladlou ironcladlou left review comments

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@fabianofranz fabianofranz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@juanvallejo @0xmichalis @deads2k @fabianofranz @openshift-merge-robot @openshift-ci-robot @openshift-bot @ironcladlou