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

Adding wildcard policy to oc create route and a column for it in oc get route #12713

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

JacobTanenbaum
Copy link
Contributor

@JacobTanenbaum JacobTanenbaum commented Jan 30, 2017

Adding wildcard policy to 'oc create route'

Adding a column for wildcard policy in 'oc get route'

Bug 1391786 Link

@JacobTanenbaum
Copy link
Contributor Author

@openshift/networking

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM @ramr PTAL

@knobunc
Copy link
Contributor

knobunc commented Jan 30, 2017

[test]

@knobunc knobunc requested a review from ramr January 30, 2017 18:31
@ncdc
Copy link
Contributor

ncdc commented Jan 30, 2017

Please reword your commit message to be descriptive as to what it's actually doing (Add wildcard policy to oc create route), and do the same for the PR title as well. Thanks!

Copy link
Contributor

@ramr ramr left a comment

Choose a reason for hiding this comment

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

LGTM

return "", fmt.Errorf("only \"Subdomain\" or \"None\" are supported for wildcardpolicy")
}
return wildcardpolicy, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should use validation from the route api (we will need to export something out in the route validation for that though). This looks good for now but if you can please add a comment here that we should use the validation from the route. Thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation functions are called as part of object creation on the server side. I used debug functions to print a stack trace on the server side on a failed attempt to create a route (for another PR but should be the same for this one) StackTrace

@knobunc
Copy link
Contributor

knobunc commented Jan 31, 2017

Ok, we will open a new bug because 'oc route' doesn't validate anything on the cli side. This PR is consistent with everything else in the 'oc route' so I think it is good to go. When this is squashed and the description cleaned up, I think it is good to go. Thanks.

@JacobTanenbaum JacobTanenbaum changed the title Completing work for BZ1391786 Adding wwildcard policy to oc create route and a column for it in oc get route Jan 31, 2017
@JacobTanenbaum JacobTanenbaum changed the title Adding wwildcard policy to oc create route and a column for it in oc get route Adding wildcard policy to oc create route and a column for it in oc get route Jan 31, 2017
@JacobTanenbaum
Copy link
Contributor Author

JacobTanenbaum commented Jan 31, 2017

The BZ that knobunc mentioned above BZ1418023

…wildcardpolicy to `oc get route'

Adding wildcard policy to 'oc create route'

Adding a column for wildcard policy in 'oc get route'

Bug 1391786
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5dbe228

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13440/) (Base Commit: ed1d332)

@ramr
Copy link
Contributor

ramr commented Feb 1, 2017

@knobunc adding some of these checks on client side could be problematic. One is that older cli clients would behave differently vis-a-vis newer cli clients + what happens when the server validation changes say for a list of choices from [foo, bar] to [foo, bar, baz] or even [one, two]? The older cli client checks would be invalid at that point or worse yet even block valid values.

@knobunc
Copy link
Contributor

knobunc commented Feb 2, 2017

@ramr: All good points. Added your concerns to the bug.

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5dbe228

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 2, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13514/) (Base Commit: 00fbbdd) (Image: devenv-rhel7_5838)

@openshift-bot openshift-bot merged commit 89ed457 into openshift:master Feb 2, 2017
@@ -112,6 +113,11 @@ func CreateEdgeRoute(f *clientcmd.Factory, out io.Writer, cmd *cobra.Command, ar
return err
}

wildcardpolicy := kcmdutil.GetFlagString(cmd, "wildcardpolicy")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be wildcard-policy. Please open a follow-up.

@@ -36,7 +36,7 @@ var (
imageStreamImageColumns = []string{"NAME", "DOCKER REF", "UPDATED", "IMAGENAME"}
imageStreamColumns = []string{"NAME", "DOCKER REPO", "TAGS", "UPDATED"}
projectColumns = []string{"NAME", "DISPLAY NAME", "STATUS"}
routeColumns = []string{"NAME", "HOST/PORT", "PATH", "SERVICES", "PORT", "TERMINATION"}
routeColumns = []string{"NAME", "HOST/PORT", "PATH", "SERVICES", "PORT", "TERMINATION", "WILDCARDPOLICY"}
Copy link
Contributor

Choose a reason for hiding this comment

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

WILDCARD, column length is critical and the extra POLICY doesn't help much.

@smarterclayton
Copy link
Contributor

Please ensure @openshift/cli-review is included on all CLI changes to enforce consistency for our processes.

@JacobTanenbaum
Copy link
Contributor Author

@smarterclayton posted follow up: #12774

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

Successfully merging this pull request may close these issues.

6 participants