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: 58466: tolerate more than one gvklist item #18157

Merged
merged 2 commits into from
Jan 23, 2018
Merged

UPSTREAM: 58466: tolerate more than one gvklist item #18157

merged 2 commits into from
Jan 23, 2018

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Jan 18, 2018 •
edited
Loading

Fixes #17872
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536845
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1529447

Origin resources, like deployment configs, are part of two api groups -
the legacy "empty" group, and, in cases like deploymentconfigs, the
"apps.openshift.io" group.

Before

$ oc explain dc
error: Couldn't find resource for "/v1, Kind=DeploymentConfig"

After

$ oc explain dc
DESCRIPTION:
     Deployment Configs define the template for a pod and manages deploying new
     images or configuration changes. A single deployment configuration is
     usually analogous to a single micro-service. Can support many different
     deployment patterns, including full restart, customizable rolling updates,
...

cc @deads2k @soltysh

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 18, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 18, 2018
soltysh
soltysh approved these changes Jan 19, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

lgtm, let's wait for the upstream to approve it and this can merge afterwards as well.

@juanvallejo juanvallejo force-pushed the jvallejo/update-openapi-ext-gvk-parsing branch from d34c70d to dbfa23f Compare January 19, 2018 19:12
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 19, 2018
@juanvallejo
Copy link
Contributor Author

Updated to keep on par with changes in upstream PR

@juanvallejo
UPSTREAM: 58466: tolerate more than one gvklist item
845259a 
Origin resources, like deployment configs, are part of two api groups -
the legacy "empty" group, and, in cases like deploymentconfigs, the
"apps.openshift.io" group.
@juanvallejo juanvallejo force-pushed the jvallejo/update-openapi-ext-gvk-parsing branch from dbfa23f to 845259a Compare January 19, 2018 19:42
@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install

@juanvallejo
Copy link
Contributor Author

@soltysh upstream has merged, ptal
kubernetes/kubernetes#58466

@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install

1 similar comment
@juanvallejo
Copy link
Contributor Author

/test extended_conformance_install

@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo
Copy link
Contributor Author

/test cmd

1 similar comment
@juanvallejo
Copy link
Contributor Author

/test cmd

@juanvallejo juanvallejo force-pushed the jvallejo/update-openapi-ext-gvk-parsing branch from 2ac29e5 to 2e29c9c Compare January 22, 2018 23:38
@mfojtik
Copy link
Contributor

mfojtik commented Jan 23, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2018
@mfojtik
Copy link
Contributor

mfojtik commented Jan 23, 2018

/retest

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, mfojtik

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2018
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit cf406ea into openshift:master Jan 23, 2018
@juanvallejo juanvallejo deleted the jvallejo/update-openapi-ext-gvk-parsing branch January 23, 2018 19:31
php-coder
php-coder reviewed Jan 24, 2018
View reviewed changes
test/cmd/explain.sh

# Cleanup cluster resources created by this test
(
set +e
Copy link
Contributor

Choose a reason for hiding this comment

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

Why set +e if we it can be written as:

oc delete all,templates --all || :

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the pattern seen in other tests: https://github.com/openshift/origin/blob/master/test/cmd/builds.sh#L7

php-coder
php-coder reviewed Jan 24, 2018
View reviewed changes
test/cmd/explain.sh
) &>/dev/null


os::test::junit::declare_suite_start "cmd/explain"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use single quotes here, for consistency with other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We seem to use double quotes in other tests [1][2][3][4]

  1. https://github.com/openshift/origin/blob/master/test/cmd/builds.sh#L16
  2. https://github.com/openshift/origin/blob/master/test/cmd/annotations.sh#L13
  3. https://github.com/openshift/origin/blob/master/test/cmd/deployments.sh#L13
  4. https://github.com/openshift/origin/blob/master/test/cmd/env.sh#L5

@soltysh soltysh mentioned this pull request Jan 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@php-coder php-coder php-coder left review comments

@soltysh soltysh soltysh approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@mfojtik mfojtik

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
6 participants
@juanvallejo @mfojtik @openshift-ci-robot @openshift-merge-robot @php-coder @soltysh