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

UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands #11733

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Nov 2, 2016

UPSTREAM: kubernetes/kubernetes#36071
Related Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1390139

Related PR (adds --local and --dr-run flags to oc set resources): #11694

Partially addresses: #7899

some oc set ... sub-commands do not implement --dry-run or --local
options. This patch adds these missing options to all sub-commands and
ensures that backwards compatibility remains for commands that treat the
--output option as a ``dry-run flag.

Example
$ oc set image dc dctest dctest-1=default/testimage:latest --dry-run
NAME      REVISION   DESIRED   CURRENT   TRIGGERED BY
dctest    1          1         1         config

$ oc set image dc dctest dctest-1=default/testimage:latest -o name
deploymentconfig/dctest

$ oc set build-secret mysecret --push --all -o
custom-columns=NAME:.metadata.name
NAME
gitauthtest
repo-base

$ oc set build-secret mysecret --push --all --dry-run
NAME          TYPE      FROM      LATEST
gitauthtest   Source    Git       1
NAME        TYPE      FROM      LATEST
repo-base   Docker    Git       8
Affected commands

cc @openshift/cli-review

@@ -134,13 +134,11 @@ func (o *ImageOptions) Complete(f *cmdutil.Factory, cmd *cobra.Command, args []s
ContinueOnError().
NamespaceParam(cmdNamespace).DefaultNamespace().
FilenameParam(enforceNamespace, o.Recursive, o.Filenames...).
SelectorParam(o.Selector).
ResourceTypeOrNameArgs(o.All, o.Resources...).
Latest().
Copy link
Contributor

Choose a reason for hiding this comment

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

Latest() does a server fetch, right? I thought the point of Local was to avoid that

@juanvallejo juanvallejo changed the title UPSTREAM: 0000: fix local resource output UPSTREAM: 0000: oc set image fix local resource output Nov 2, 2016
@juanvallejo
Copy link
Contributor Author

@liggitt Thanks for the feedback, review comment addressed

@ncdc
Copy link
Contributor

ncdc commented Nov 2, 2016

Needs an actual upstream PR. Can this wait to come in via rebase? cc @pweil- @mfojtik

@juanvallejo juanvallejo changed the title UPSTREAM: 0000: oc set image fix local resource output UPSTREAM: 36071: oc set image fix local resource output Nov 2, 2016
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch 2 times, most recently from 2d0a1c5 to 24bbf78 Compare November 2, 2016 19:21
@mfojtik
Copy link
Contributor

mfojtik commented Nov 3, 2016

@ncdc i'm fine waiting for the rebase

@juanvallejo
Copy link
Contributor Author

[test]

@juanvallejo
Copy link
Contributor Author

conformance check flaked on #10988 re[test]

@juanvallejo juanvallejo changed the title UPSTREAM: 36071: oc set image fix local resource output UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands Nov 4, 2016
@juanvallejo juanvallejo changed the title UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands [WIP] UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands Nov 4, 2016
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from 24bbf78 to 8ccf43b Compare November 4, 2016 22:32
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch 5 times, most recently from 5cb6a2a to 8990f32 Compare November 10, 2016 18:22
@juanvallejo
Copy link
Contributor Author

make verify flaked on #11672 re[test]

@fabianofranz
Copy link
Member

The last failure complained about outdated docs, did you hack/update-generated-docs.shrecently?

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Nov 10, 2016

@fabianofranz Yeah, I updated docs right after that failure, however it has been failing with tput: No value for $TERM and no -T specified since then. (Although locally it fails with:

===== Verifying Generated Bootstrap Bindata =====
Generating bootstrap bindata...
FAILURE: Generation of fresh bindata failed:
/home/jvallejo/Documents/go/src/github.com/openshift/origin/hack/update-generated-bootstrap-bindata.sh: line 18: : command not found
[ERROR] PID 16909: hack/update-generated-bootstrap-bindata.sh:18: `"$(os::util::find-go-binary go-bindata)" -nocompress -nometadata -prefix "bootstrap" -pkg "bootstrap" -o "${OUTPUT_PARENT}/pkg/bootstrap/bindata.go" -ignore "README.md" -ignore ".*\.go$" -ignore application-template.json ${EXAMPLES}/image-streams/... ${EXAMPLES}/db-templates/... ${EXAMPLES}/jenkins ${EXAMPLES}/jenkins/pipeline ${EXAMPLES}/quickstarts/... ${EXAMPLES}/logging/... pkg/image/admission/imagepolicy/api/v1/...` exited with status 127.

)

@juanvallejo
Copy link
Contributor Author

@stevekuznetsov I keep getting tput: No value for $TERM and no -T specified on the Travis test, and
this locally when running GO_PERMISSIVE=y make verify

===== Verifying Generated Bootstrap Bindata =====
Generating bootstrap bindata...
FAILURE: Generation of fresh bindata failed:
/home/jvallejo/Documents/go/src/github.com/openshift/origin/hack/update-generated-bootstrap-bindata.sh: line 18: : command not found
[ERROR] PID 16909: hack/update-generated-bootstrap-bindata.sh:18: `"$(os::util::find-go-binary go-bindata)" -nocompress -nometadata -prefix "bootstrap" -pkg "bootstrap" -o "${OUTPUT_PARENT}/pkg/bootstrap/bindata.go" -ignore "README.md" -ignore ".*\.go$" -ignore application-template.json ${EXAMPLES}/image-streams/... ${EXAMPLES}/db-templates/... ${EXAMPLES}/jenkins ${EXAMPLES}/jenkins/pipeline ${EXAMPLES}/quickstarts/... ${EXAMPLES}/logging/... pkg/image/admission/imagepolicy/api/v1/...` exited with status 127.

was hack/update-generated-bootstrap-bindata.sh being run before?

@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from 8990f32 to c588461 Compare November 10, 2016 20:37
@stevekuznetsov
Copy link
Contributor

@juanvallejo travis bug is fixed with #11877

@fabianofranz
Copy link
Member

flaked on #10228 and #11771 and #11876 re[test]

@ncdc
Copy link
Contributor

ncdc commented Nov 11, 2016

Closing as we'll get this with the kube 1.5 rebase.

@ncdc ncdc closed this Nov 11, 2016
@juanvallejo juanvallejo reopened this Nov 11, 2016
@juanvallejo
Copy link
Contributor Author

Reopening as the kube rebase won't give us fixes for the rest of the oc set... subcommands

@fabianofranz
Copy link
Member

@juanvallejo need to squash the local and upstream commits.

@ncdc
Copy link
Contributor

ncdc commented Dec 9, 2016

Since this is after the 1.6 rebase, there's no need to squash or do anything else with this now. Once Kube 1.6 comes in, the UPSTREAM commits will come out of this PR.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2016
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch 2 times, most recently from 24616f8 to 9ea1437 Compare January 11, 2017 21:39
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch 3 times, most recently from 43d46b7 to 4f74c43 Compare January 12, 2017 20:24
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch 2 times, most recently from d889f70 to 73cbf3a Compare February 27, 2017 15:29
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@fabianofranz fabianofranz changed the title UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands [POST-REBASE] UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands Mar 2, 2017
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2017
@juanvallejo juanvallejo changed the title [POST-REBASE] UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands UPSTREAM: 36071: Add --local & --dry-run flags to oc set sub-commands May 4, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from c598f22 to e016c56 Compare May 4, 2017 22:28
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from e016c56 to 770d4c4 Compare May 8, 2017 17:35
@fabianofranz
Copy link
Member

@juanvallejo legit failure?

FAILURE after 0.920s: test/cmd/basicresources.sh:208: executing 'oc set probe dc test-deployment-config --liveness --open-tcp=8080' expecting success and text 'updated': the command returned the wrong error code; the output content test failed

@juanvallejo
Copy link
Contributor Author

@fabianofranz hm, I can't seem to reproduce this locally, rebasing with latest master and once more

@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from 770d4c4 to d49772c Compare May 9, 2017 23:43
some `oc set ...` sub-commands do not implement `--dry-run` or `--local`
options. This patch adds these missing options to all sub-commands and
ensures that backwards compatibility remains for commands that treat the
--output option as a ``dry-run flag.

```
$ oc set image dc dctest dctest-1=default/testimage:latest --dry-run
NAME      REVISION   DESIRED   CURRENT   TRIGGERED BY
dctest    1          1         1         config

$ oc set image dc dctest dctest-1=default/testimage:latest -o name
deploymentconfig/dctest

$ oc set build-secret mysecret --push --all -o
custom-columns=NAME:.metadata.name
NAME
gitauthtest
repo-base

$ oc set build-secret mysecret --push --all --dry-run
NAME          TYPE      FROM      LATEST
gitauthtest   Source    Git       1
NAME        TYPE      FROM      LATEST
repo-base   Docker    Git       8
```
@juanvallejo juanvallejo force-pushed the jvallejo/fix-oc-set-image-local-output branch from d49772c to 16c4978 Compare May 10, 2017 02:11
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 16c4978

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1283/) (Base Commit: 31caa0d)

@juanvallejo juanvallejo deleted the jvallejo/fix-oc-set-image-local-output branch May 10, 2017 04:06
@juanvallejo
Copy link
Contributor Author

@fabianofranz meant to force-push after updating set-image.sh, set-liveness-probe.sh, but deleted branch on accident instead. Apparently I cannot restore the branch once I have force pushed after its deletion.

Re-opened here: #14123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants