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.

Sign up for GitHub

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

Jump to bottom

feat(setting): change ui-pl and ui-brand by rancher-manager-support #3784

Merged
merged 2 commits into from
Jun 1, 2023
Merged

feat(setting): change ui-pl and ui-brand by rancher-manager-support #3784

merged 2 commits into from
Jun 1, 2023

Conversation

FrankYang0529
Copy link
Member

@FrankYang0529 FrankYang0529 commented Apr 18, 2023 •
edited by futuretea
Loading

Solution:
Fix dashboard brand.

Related Issue:
#2679

Test plan:

  1. Patch harvester image or create new cluster from iso
  2. Show Harvester logo and Harvester label in the web brower tab
  3. Update rancher-manager-support to true.
  4. Show Rancher logo and Rancher label in the web brower tab
  5. Update rancher-manager-support to false.
  6. Show Harvester logo and Harvester label in the web brower tab

@futuretea
Copy link
Contributor

find some code about ui-pl:

harvester/pkg/controller/master/rancher/embedded_rancher.go

Line 16 in 231a496

var UpdateRancherUISettings = map[string]string{

@FrankYang0529
Copy link
Member Author

harvester/pkg/controller/master/rancher/embedded_rancher.go

Line 16 in 231a496

var UpdateRancherUISettings = map[string]string{

This part is about updating Default value.

@FrankYang0529 FrankYang0529 changed the title feat(setting): change ui-pl by rancher-manager-support feat(setting): change ui-pl and ui-brand by rancher-manager-support Apr 19, 2023
@ibrokethecloud
Copy link
Contributor

should we document that while using embedded rancher in mcm mode, the end users cannot change the ui-pl or ui-brand settings?

ibrokethecloud
ibrokethecloud reviewed May 19, 2023
View reviewed changes
pkg/controller/master/setting/rancher_manager_support.go
}

uiPLCopy := uiPL.DeepCopy()
if enable {
Copy link
Contributor

Choose a reason for hiding this comment

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

the default value of the setting is already rancher and is applied with left empty.

should we following the same logic as we do for ui-brand and leave it empty when enabled?

On an existing install they are already empty.

(⎈|local:N/A)➜  harvester git:(update-ui-pl) ✗ k get settings.management ui-pl ui-brand
NAME       VALUE
ui-pl
ui-brand

Copy link
Contributor

@futuretea futuretea May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the default value of ui-pl in Harvester is patched to "Harvester" in

harvester/pkg/controller/master/rancher/embedded_rancher.go

Line 33 in 29bcdcc

for name, value := range UpdateRancherUISettings {
.

Copy link
Contributor

@futuretea futuretea May 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The UI currently uses this ui-pl default value "Harvester" to decide whether to display the bootstrap password box on the first login.
I will communicate with @WuJun2016 and create an issue to deal with the default value of ui-pl.

@WuJun2016
Copy link
Contributor

Associated UI pr and feature: #3396

@WuJun2016
Copy link
Contributor

WuJun2016 commented May 23, 2023 •
edited
Loading

should we document that while using embedded rancher in mcm mode, the end users cannot change the ui-pl or ui-brand settings?

@ibrokethecloud ui-pl and ui-brand should be customizable by the users. As shown in the diagram below, users may want to differentiate different clusters by customizing the names.

harvester page:
image

rancher page:
image

@guangbochen guangbochen mentioned this pull request May 29, 2023
Closed
@FrankYang0529 @futuretea
feat(setting): change ui-pl and ui-brand by rancher-manager-support
7342bc4 
Signed-off-by: Frank Yang <poan.yang@suse.com>
@futuretea futuretea marked this pull request as draft May 30, 2023 09:51
@futuretea futuretea self-assigned this May 31, 2023
@futuretea futuretea marked this pull request as ready for review May 31, 2023 02:14
@futuretea
Bootstrap ui-brand and ui-pl
ea31cf2 
Signed-off-by: futuretea <Hang.Yu@suse.com>
@futuretea
Copy link
Contributor

Add a patch commit to this PR, need review again.

ibrokethecloud
ibrokethecloud approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@ibrokethecloud ibrokethecloud left a comment

Choose a reason for hiding this comment

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

thanks for the PR. LGTM.

bk201
bk201 approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for continuing the PR.

guangbochen
guangbochen approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@guangbochen guangbochen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@guangbochen guangbochen merged commit aec02d9 into harvester:master Jun 1, 2023
@WuJun2016 WuJun2016 mentioned this pull request Jul 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bk201 bk201 bk201 approved these changes

@guangbochen guangbochen guangbochen approved these changes

@futuretea futuretea futuretea approved these changes

@ibrokethecloud ibrokethecloud ibrokethecloud approved these changes

Assignees

@futuretea futuretea

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@FrankYang0529 @futuretea @ibrokethecloud @WuJun2016 @bk201 @guangbochen