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

centraldashboard: Support dynamic logout URL #6945

Conversation

orfeas-k
Copy link
Contributor

@orfeas-k orfeas-k commented Feb 10, 2023

Use dynamic URL for the logout button that can be set by the LOGOUT_URL ENV variable in the deployment of the app. If it is unset, a default '/logout' string is passed from the backend to the frontend.

I documented in the issue here details on how I dealt with binding data between Polymer elements.

During this PR, we also had to fix a test that was broken. It seems that the test was failing because new Date () added whitespace that was not equal to the space character during comparison. Instead of comparing objects of type Date and string in the expect statement, we went with comparing Date objects in order to ensure comparison in a uniform and type-strict way.

Use dynamic URL for the logout button that can be set by the LOGOUT_URL
ENV variable in the deployment of the app. If it is unset, a default
'/logout' string is passed from the backend to the frontend.

Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
@orfeas-k orfeas-k force-pushed the feature-orfeas-centraldashboard-support-dynamic-logout-urls branch from e105b76 to 9772882 Compare February 13, 2023 09:32
@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Feb 13, 2023
@orfeas-k orfeas-k changed the title [WIP] centraldashboard: Support dynamic logout URL centraldashboard: Support dynamic logout URL Feb 13, 2023
@kimwnasptd
Copy link
Member

Thanks for the PR @orfeas-k! Could you send the commit for fixing the tests as a dedicated PR?

Looks like we are having some build issues when PRs are merged and I'd like to tackle this as a dedicated effort
74f020e
65e41bf

@orfeas-k orfeas-k force-pushed the feature-orfeas-centraldashboard-support-dynamic-logout-urls branch from 9772882 to 9356553 Compare February 15, 2023 12:28
@orfeas-k
Copy link
Contributor Author

Sure, I also removed the tests commit from this PR

@kimwnasptd
Copy link
Member

@orfeas-k tested the PR and while the logout works, I end up seeing a blank screen. Navigating back to the root URL gets me to login, so the logout functionality works, but I think something might not be configured correctly on the AuthService's side.

I'll look more into it with @athamark. @DomFleischmann there's a chance we'll need some fix for this, but we'll comment back once we have more news

@kimwnasptd
Copy link
Member

I'll go on and merge this for now to include this in the RC1

/lgtm
/approve

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

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

@google-oss-prow google-oss-prow bot merged commit 80539fc into kubeflow:master Feb 15, 2023
@kimwnasptd kimwnasptd deleted the feature-orfeas-centraldashboard-support-dynamic-logout-urls branch February 15, 2023 18:16
kimwnasptd pushed a commit to arrikto/kubeflow that referenced this pull request Feb 16, 2023
Use dynamic URL for the logout button that can be set by the LOGOUT_URL
ENV variable in the deployment of the app. If it is unset, a default
'/logout' string is passed from the backend to the frontend.

Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
google-oss-prow bot pushed a commit that referenced this pull request Feb 16, 2023
Use dynamic URL for the logout button that can be set by the LOGOUT_URL
ENV variable in the deployment of the app. If it is unset, a default
'/logout' string is passed from the backend to the frontend.

Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
Co-authored-by: Orfeas Kourkakis <orfeas@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

2 participants