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
fixes 'kfctl needs to support passing swagger spec to ks init from file' #2719
Conversation
/assign @jlewi |
/assign @gabrielwen |
/retest |
Marking this WIP until blocking PRs are merged. |
c481926
to
935be96
Compare
pick f2a218c fixes 'kfctl needs to support passing swagger spec to ks init from file'
935be96
to
d0897a2
Compare
/assign @kunmingg |
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.
Reviewed 46 of 51 files at r1, 1 of 1 files at r2.
Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @ashahba, @ellis-bigelow, and @kkasravi)
bootstrap/cmd/kfctl/cmd/root.go, line 46 at r1 (raw file):
var rootCmd = &cobra.Command{ Use: "kfctl", Short: "A kfdef tool to create kubeflow applications",
kfdef
seems like a jargon?
bootstrap/pkg/kfapp/coordinator/coordinator.go, line 217 at r1 (raw file):
if strings.Compare(validName, appName) != 0 { return nil, fmt.Errorf(`invalid name %v must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character`, appName)
should we also check the length of app name?
bootstrap/pkg/apis/apps/kfdef/v1alpha1/application_types.go, line 34 at r1 (raw file):
Hostname string `json:"hostname,omitempty"` Zone string `json:"zone,omitempty"` BasicAuthUsername string `json:"basicAuthUsername,omitempty"`
let's remove username/password from spec as user might accidentally check in sensitive information into source control. current logic is to read from ENV.
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.
Reviewable status: 2 of 7 files reviewed, 3 unresolved discussions (waiting on @ashahba, @ellis-bigelow, and @gabrielwen)
bootstrap/cmd/kfctl/cmd/root.go, line 46 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
kfdef
seems like a jargon?
updated to something more poetic
bootstrap/pkg/kfapp/coordinator/coordinator.go, line 217 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
should we also check the length of app name?
delegated to apimachinery validation of a DNSLabel
bootstrap/pkg/apis/apps/kfdef/v1alpha1/application_types.go, line 34 at r1 (raw file):
Previously, gabrielwen (Hung-Ting Wen) wrote…
let's remove username/password from spec as user might accidentally check in sensitive information into source control. current logic is to read from ENV.
Done.
/lgtm |
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.
thanks
Reviewable status: 2 of 7 files reviewed, 3 unresolved discussions (waiting on @ashahba, @ellis-bigelow, and @gabrielwen)
bootstrap/cmd/kfctl/cmd/root.go, line 46 at r1 (raw file):
Previously, kkasravi (Kam Kasravi) wrote…
updated to something more poetic
Done.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lluunn 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 |
…le' (kubeflow#2719) * fixes 'kfctl needs to support passing swagger spec to ks init from file' pick f2a218c fixes 'kfctl needs to support passing swagger spec to ks init from file' * fix in ksonnet for passing config if not gcp * update kfctl short, long usage * update with suggestions from Gabriel's review
…le' (kubeflow#2719) * fixes 'kfctl needs to support passing swagger spec to ks init from file' pick f2a218c fixes 'kfctl needs to support passing swagger spec to ks init from file' * fix in ksonnet for passing config if not gcp * update kfctl short, long usage * update with suggestions from Gabriel's review
fixes #2706, #2709
This change is