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

make IAP optional for click-deploy app #1927

Merged
merged 5 commits into from Nov 20, 2018
Merged

Conversation

kunmingg
Copy link
Contributor

@kunmingg kunmingg commented Nov 8, 2018

  1. make IAP optional for click-deploy app
  2. update instructions for both cases (IAP / no IAP)
  3. add button for both cases to smooth experience

This change is Reviewable

</ul>
<ul>
<ul>
<li> Click IAP Access (might need to wait up to 20 minutes on network settings) </li>
Copy link
Contributor

@jlewi jlewi Nov 8, 2018

Choose a reason for hiding this comment

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

"20 minutes for domain and IAP to be setup"

</ul>
<ul>
<ul>
<li> Click Cloud Shell; run command in pop up window; kubeflow UI will be at
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify these instructions? Where do users "click Cloud Shell"? Can we provide a link to launch it?
What command are they supposed to run in Cloud Shell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We provide button on UI and prepare script in user's source repo,
when user click button, cloud shell will clone repo and cd into script dir.
User only need to run ./conn.sh

@@ -262,12 +271,53 @@ export default class DeployForm extends React.Component<any, DeployFormState> {
if (p.name === 'acmeEmail') {
p.value = email;
}
});

if (p.name === 'jupyterHubAuthenticator') {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the client side JS is responsible for setting the authenticator for JupyterHub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally not, will take care in #1971

@jlewi
Copy link
Contributor

jlewi commented Nov 8, 2018

This is great; just a minor comment about the instructions.

@abhi-g
Copy link
Member

abhi-g commented Nov 12, 2018

Is it possible to add a checkbox for IAP setup, which is unchecked by default. When unchecked the IAP related fields can be hidden from the form, and visible if checked.

@kunmingg kunmingg force-pushed the connect branch 2 times, most recently from 7c5b7de to c2a9076 Compare November 13, 2018 07:19
@jlewi
Copy link
Contributor

jlewi commented Nov 15, 2018

/lgtm but looks like this is now out of sync and needs to be updated.

@kunmingg
Copy link
Contributor Author

@jlewi
For UI access part, this PR only pre-generate script & instruction which semi-automate port-forward.
As port-forward only establish temp connection and could cause some bugs (like kubeflow/pipelines#179).
Should we spend some time on ambassador AuthService?

@jlewi
Copy link
Contributor

jlewi commented Nov 16, 2018

@kunmingg Yes; I think using Auth Ambassador service with basic http auth is probably a better option.

I still think we should probably go forward with this PR so that users can avoid enabling IAP and just use kubectl port-forward. WDYT?

@abhi-g
Copy link
Member

abhi-g commented Nov 16, 2018 via email

@kunmingg
Copy link
Contributor Author

Sure, seems we might be able to automate cloud shell.
Will change frontend a bit and merge this PR.

@jlewi
Copy link
Contributor

jlewi commented Nov 20, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

@k8s-ci-robot k8s-ci-robot merged commit d72ba77 into kubeflow:master Nov 20, 2018
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
* make IAP optional for click-deploy app

* more

* automate cloud shell

* frontend edit following review feedbacks

* add input check when user choose to setup IAP
surajkota pushed a commit to surajkota/kubeflow that referenced this pull request Jun 13, 2022
Signed-off-by: Yannis Zarkadas <yanniszark@arrikto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants