-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improvement/2016 brand the dex gui to meet the design #2062
Improvement/2016 brand the dex gui to meet the design #2062
Conversation
Hello chengyanjin,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/after_pull_request=2063 |
Waiting for other pull request(s)The current pull request is locked by the after_pull_request option. In order for me to merge this pull request, run the following actions first: ➡️ Merge the
Alternatively, delete all the after_pull_request comments from this pull request. The following options are set: after_pull_request |
/after_pull_request=2025 |
Waiting for other pull request(s)The current pull request is locked by the after_pull_request option. In order for me to merge this pull request, run the following actions first: ➡️ Merge the
Alternatively, delete all the after_pull_request comments from this pull request. The following options are set: after_pull_request |
@ChengYanJin you'll need to rebase on |
ac37a10
to
ab3423d
Compare
Waiting for other pull request(s)The current pull request is locked by the after_pull_request option. In order for me to merge this pull request, run the following actions first: ➡️ Merge the
Alternatively, delete all the after_pull_request comments from this pull request. The following options are set: after_pull_request |
620f955
to
cd5391f
Compare
cd5391f
to
ac4518d
Compare
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.
LGTM for the buildchain part.
I tested it and it works.
Good job 👍
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.
Minor changes, good job! We'll need to think about a different approach though (I guess fetch the assets from MetalUI directly could work, so that if the theme changes, this one will as well)
def salt_embed_text(path: Path, indent: int) -> str: | ||
"""Return the content a text file in usable form by Salt.""" | ||
data = ['|'] | ||
with path.open(encoding='utf-8') as fp: | ||
for line in fp: | ||
data.append('{}{}'.format(' '*indent, line.rstrip())) | ||
return '\n'.join(data) | ||
|
||
|
||
def salt_embed_bytes(path: Path, indent: int) -> str: | ||
"""Return the content of a binary file in usable form by Salt.""" | ||
data = ['|'] | ||
bytestring = path.read_bytes() | ||
b64data = base64.encodebytes(bytestring).decode('utf-8') | ||
for line in b64data.split('\n'): | ||
data.append('{}{}'.format(' '*indent, line.rstrip())) | ||
return '\n'.join(data) |
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.
Mmpf, that's a strange way to do things. Both could rely on yaml.dump
(see 1ada06c for an example of how to customize representers). But let's keep it for a debt ticket.
The three files will be mounted in web/themes/scality in order to render our customized theme Refs: #2016
ac4518d
to
824d2cb
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: The following options are set: after_pull_request |
Hi Guillaume, thanks for the review! Actually I fetch different asset for Dexlogin and Metalk8sUI. e.g For scality logo, we use branding.svg => MetalK8s and logo.png => Dexlogin.
I moved the dexlogin assets in the ui/public/brand/assets/login. Wondering does it make sense to you? |
824d2cb
to
572d0d7
Compare
Oh OK. Actually my point was that, instead of mounting those assets in the Dex container, we could find some way to let them be referenced by URL instead (such URL pointing to Metal UI). But really not urgent. I think I've seen a
Yeah it will make sense if we manage to serve them with Metal UI (and not store them in a ConfigMap at build time). So let's keep it, good idea :) |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: after_pull_request |
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, after_pull_request |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye chengyanjin. |
extraVolumes: | ||
- name: dex-login | ||
configMap: | ||
name: dex-login |
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.
I wish we'd have given this a less generic name like dex-scality-theme
or whatever.
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.
I wish we'd have given this a less generic name like
dex-scality-theme
or whatever.
I will rename it in the 2nd iteration of authentication.
Don't postpone renaming of things. It's a hassle because of extra
upgrade/downgrade automation required.
…On Thu, 19 Dec 2019, 16:27 ChengYanJin, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In charts/dex.yaml
<#2062 (comment)>:
> @@ -31,14 +31,14 @@ ingress:
hosts:
- null
-# extraVolumes:
-# - name: theme
-# configMap:
-# name: dex-branding
+extraVolumes:
+ - name: dex-login
+ configMap:
+ name: dex-login
I wish we'd have given this a less generic name like dex-scality-theme or
whatever.
I will rename it in the 2nd iteration of authentication.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2062?email_source=notifications&email_token=AAAZBTO3QWYWNRXJJBYRZHLQZOHGPA5CNFSM4JQDV2TKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPZUBKQ#discussion_r359918296>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAZBTKKPSDEEPXTHK5U7FTQZOHGPANCNFSM4JQDV2TA>
.
|
Component: Dex, UI
Context:
As I user, I want to have the same user experience as Metalk8s UI when I log in.
Summary:
We create a configMap and mount it as volume in the web/theme/scality
Acceptance criteria:
https://<control_plane_ip>:8443/oidc/auth?audience=&client_id=metalk8s-ui&redirect_uri=https%3A%2F%2F<control_plane_ip>%3A8443%2Foauth2%2Fcallback&response_type=id_token&scope=openid+profile+email+offline_access&nonce=abcde
Closes: #2016