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
Move basic auth username/password to ENV #2713
Conversation
/assign @jlewi |
/assign @kunmingg |
@gabrielwen Can you look at why the test failed and open an issue if the failure is unrelated to this PR? |
Can we pass them during apply instead of save as ENV? |
/test kubeflow-presubmit |
failure doesn't make sense to me, rerunning it.
|
seems like something wrong with deepcopy:
|
Issue filed: #2715 |
/retest |
Fixes #2715 |
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.
#2705 E2E test should be submitted first. That way the test can then be used to verify this PR is working.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, and @kkasravi)
bootstrap/cmd/kfctl/cmd/init.go, line 56 at r2 (raw file):
if useBasicAuth && (os.Getenv(kftypes.BASIC_AUTH_USERNAME) == "" || os.Getenv(kftypes.BASIC_AUTH_PASSWORD) == "") { // Printing warning message instead of bailing out as both ENV are used in apply,
The comment isn't clear to me. Can you explain?
If we only need these env for apply and not init; can we check what the command is and only error out if we are running the command that requires it?
bootstrap/cmd/kfctl/cmd/init.go, line 58 at r2 (raw file):
// Printing warning message instead of bailing out as both ENV are used in apply, // not init. log.Warnf("If using basic auth, need to set ENV %v and %v when running kfctl apply",
"you need to set the environment variable %s to the username you want to use to login and variable %s to the password you want to use."
bootstrap/pkg/apis/apps/group.go, line 51 at r2 (raw file):
DefaultGkeApiVer = "v1beta1" DefaultAppLabel = "app.kubernetes.io/name" BASIC_AUTH_USERNAME = "BASIC_AUTH_USERNAME"
Lets call the environment variables
KUBEFLOW_USERNAME
KUBEFLOW_PASSWORD
/retest |
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.
#2705 Is merged. Can you rebase? You'll need to update the test to work with this change.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, and @kkasravi)
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: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ellis-bigelow, @jlewi, and @kkasravi)
bootstrap/cmd/kfctl/cmd/init.go, line 56 at r2 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
The comment isn't clear to me. Can you explain?
If we only need these env for apply and not init; can we check what the command is and only error out if we are running the command that requires it?
this is merely a friendly reminder to set the ENV variables.
bootstrap/cmd/kfctl/cmd/init.go, line 58 at r2 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
"you need to set the environment variable %s to the username you want to use to login and variable %s to the password you want to use."
Done.
bootstrap/pkg/apis/apps/group.go, line 51 at r2 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
Lets call the environment variables
KUBEFLOW_USERNAME
KUBEFLOW_PASSWORD
Done.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kunmingg 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 |
* disable kustomize * remove username/password from params * remove CliOption * add check in init * use ENV * remove from spec * revert comment * add GOPATH to fix test * rebase * update test for ENV username/password * add some logs * update * update * update naming
* disable kustomize * remove username/password from params * remove CliOption * add check in init * use ENV * remove from spec * revert comment * add GOPATH to fix test * rebase * update test for ENV username/password * add some logs * update * update * update naming
Relates to #2661
This change is