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

add --dry-run option to "oc adm groups new" #19811

Conversation

juanvallejo
Copy link
Contributor

Adds a --dry-run option to oc adm groups new
Fixes #14807

cc @soltysh

@juanvallejo juanvallejo requested a review from soltysh May 22, 2018 21:53
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 22, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from cd49de5 to c3bfddc Compare May 22, 2018 21:57
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from c3bfddc to 5e69459 Compare May 23, 2018 20:58
Copy link
Member

@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
/approve
You'll need to update completions and figure out why that new cmd test fails (line 121), it's weird though ;)

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 24, 2018
@openshift-bot
Copy link
Contributor

/retest

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

@@ -117,6 +117,11 @@ echo "certs: ok"
os::test::junit::declare_suite_end

os::test::junit::declare_suite_start "cmd/admin/groups"
# test --dry-run flag for this command
os::cmd::expect_success_and_text 'oc adm groups new mygroup --dry-run' 'group "mygroup" created (dry run)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to escape parentheses \(dry run\)

@openshift-bot
Copy link
Contributor

/retest

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

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from 5e69459 to 706006c Compare May 24, 2018 13:48
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch 2 times, most recently from 764afbf to d1638d8 Compare May 24, 2018 17:08
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from d1638d8 to 74306d1 Compare June 29, 2018 01:03
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018
@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 Jun 29, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2018
@soltysh
Copy link
Member

soltysh commented Jul 2, 2018

@juanvallejo do want to land this before we get there with our re-factors or we'll pick it up during those?

@juanvallejo
Copy link
Contributor Author

@soltysh

do want to land this before we get there with our re-factors or we'll pick it up during those?

I think we can land it now, will rebase

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from 74306d1 to b7a249b Compare July 3, 2018 21:19
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2018
@juanvallejo
Copy link
Contributor Author

@soltysh rebased

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from b7a249b to 04fcd4f Compare July 3, 2018 22:16
@soltysh
Copy link
Member

soltysh commented Jul 4, 2018

The error:

test/integration/groups_test.go:193:10: cannot use func literal (type func("github.com/openshift/origin/vendor/k8s.io/apimachinery/pkg/runtime".Object, "io".Writer) error) as type printers.ResourcePrinter in field value:
	func("github.com/openshift/origin/vendor/k8s.io/apimachinery/pkg/runtime".Object, "io".Writer) error does not implement printers.ResourcePrinter (missing PrintObj method)

looks relevant.

Copy link
Member

@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

Feel free to re-tag once you fix that integration compile error

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from 04fcd4f to 12ffdac Compare July 6, 2018 20:11
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 6, 2018
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from 12ffdac to 9a716a4 Compare July 9, 2018 18:12
@openshift-ci-robot openshift-ci-robot added 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 9, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch 7 times, most recently from f4a0f63 to fdede35 Compare July 9, 2018 21:34
@juanvallejo
Copy link
Contributor Author

/retest

@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch 2 times, most recently from a670d96 to 5a4b961 Compare July 13, 2018 21:39
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from 5a4b961 to fe6975d Compare July 17, 2018 14:39
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2018
@juanvallejo juanvallejo force-pushed the jvallejo/add-dry-run-new-group branch from fe6975d to f915be1 Compare July 17, 2018 14:45
Copy link
Member

@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
/approve

userapi "github.com/openshift/origin/pkg/user/apis/user"
userclientinternal "github.com/openshift/origin/pkg/user/generated/internalclientset"
usertypedclient "github.com/openshift/origin/pkg/user/generated/internalclientset/typed/user/internalversion"
userapiv1 "github.com/openshift/api/user/v1"
Copy link
Member

Choose a reason for hiding this comment

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

@juanvallejo yeah, we'll need to sweep them afterwards, I want to get these pulls in asap.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2018
@soltysh soltysh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 10b4e5e into openshift:master Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants