-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Bug 1198002 - Fix broken --parameters switch for process #1218
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
Conversation
@smarterclayton @soltysh @bparees PTAL I needed to move the printing function to process command as the function is not called from printer (it was replaced with just printing the template details?). And since we are not printing the Template but just parameters, it does not fell to the 'printer.go' |
LGTM |
@@ -89,7 +110,7 @@ Examples: | |||
// If 'parameters' flag is set it does not do processing but only print | |||
// the template parameters to console for inspection. | |||
if cmdutil.GetFlagBool(cmd, "parameters") == true { | |||
err = printer.PrintObj(templateObj, out) | |||
err = printTemplateParameters(templateObj, out) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't you printing the template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you should be passing templateObj.Parameters here, and then keep printTemplateParameters in describe/printer.go (and register template parameters as its own type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton template parameters are not API objects AFAIK, I think I tried this before and the PrintObj require runtime.Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make PrintTemplateParameters public then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfojtik I don't think clayton was suggesting using printobj, just that you should pass the Parameters object, not the full template object. you're not using any other fields of the object.
also who was calling printTemplateParameters before? I see you moved it, but no other code was changed...so was nothing calling it before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, get on Template (printTemplate) called printTemplateParameters. printTemplate now displays details of a template.
----- Original Message -----
@@ -89,7 +110,7 @@ Examples:
// If 'parameters' flag is set it does not do processing but only print
// the template parameters to console for inspection.
if cmdutil.GetFlagBool(cmd, "parameters") == true {
err = printer.PrintObj(templateObj, out)
err = printTemplateParameters(templateObj, out)
@mfojtik I don't think clayton was suggesting using printobj, just that you
should pass the Parameters object, not the full template object. you're not
using any other fields of the object.also who was calling printTemplateParameters before? I see you moved it, but
no other code was changed...so was nothing calling it before?
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1218/files#r25795458
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton that makes sense, I will rework this, thanks for looking!
5248df3
to
ef6f62b
Compare
@smarterclayton @bparees moved the func into printer.go and I made it public. It now accepts list of parameters instead of template. PTAL |
LGTM [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1120/) (Image: devenv-fedora_979) |
Evaluated for origin up to ef6f62b |
…service-catalog/' changes from ef63307bdb..ae6b643caf ae6b643caf Use oc adm instead of oadm which might not exist in various installations. 66a4eb2a2c Update instructions... will remove once documented elsewhere 1b704d1530 replace build context setup with init containers ee4df18c7f hack/lib: dedup os::util::host_platform and os::build::host_platform 1cd6dfa998 origin: Switch out owners to Red Hatters 664f4d318f Add instructions for syncing repos 2f2cdd546b origin-build: delete files with colon in them cdf8b12848 origin-build: don't build user-broker ebfede9056 origin build: add _output to .gitignore 55412c7e3d origin build: make build-go and build-cross work 68c74ff4ae origin build: modify hard coded path 3d41a217f6 origin build: add origin tooling a8fc27d Fix typos in walkthrough (openshift#1224) e77edbf openshift#1157: Limit the amount of time for reconciliations (openshift#1196) 1b1a749 temporarily disabled verify-links.sh from the verify target (openshift#1226) acf8fab Send originating identity headers in OSB requests (openshift#1162) 821ba16 new admission controller to block updates to service instance updates that (openshift#1210) d69c5e5 Minor improvement to godoc in binaries (openshift#1211) 5b81814 fix typos (openshift#1221) 836dc4a Adds how to download Helm chart (charts/catalog) (openshift#1219) 2fd0115 Fix "visit the project on github" link. (openshift#1217) 325e4b6 Add how to set $GOPATH. (openshift#1218) 68b775f Update the installation (openshift#1199) 6e3a3c1 v0.0.19 (openshift#1207) 8b69791 Removing errexit from TLS setup script (openshift#1206) 273260f Instance deletion lifecycle enhancements, issue openshift#820 (openshift#1159) c050713 fix cleaning of build output for non-root users (openshift#1205) 5995df1 Merge branch 'pr/1204' 72f4802 Remove osb prefix from example ServiceClass (openshift#1201) f9dbd4e pin all dependencies in glide to current version except for glog where we want to pick up the prior version to fix issue 1187. f148bc5 v0.0.18 (openshift#1202) b86ab8d Removing the helm install command (openshift#1185) 3cff482 Remove Alpha* prefix on all API fields for issue openshift#1180 (openshift#1184) 154b74d Fix gofmt issue (openshift#1192) 2ee894a do the clean before building an arch (openshift#1179) b4976ef Fix bad URL (openshift#1189) cd3dede Fix hrefs again (openshift#1190) f066226 Design: Instance/Binding parameters (openshift#1075) eb37682 This generated file is missing from master (openshift#1191) 28c0ae7 Use generation instead of checksum for Instances and InstanceCredentials (openshift#1151) 5cdd323 Fix bad href (openshift#1188) 8a892f0 handle lingering polling cases (openshift#1174) f5fabd6 remove TPRs from Jenkins e2e pipeline (openshift#1175) 717df78 Add godoc explaining that Instance and InstanceCredential specs are immutable (openshift#1182) REVERT: ef63307bdb origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: ae6b643cafd3a17412f173e70ed7c1a2e39ee549
We need an instruction how to set $GOPATH in bash.
[3.9] Make DNS to the local node IP bypass auto-egress-IP routing
No description provided.