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

Support CYMBAL_BRANDING environment variable #602

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

NimJay
Copy link
Collaborator

@NimJay NimJay commented Sep 24, 2021

See the "What needs to be done?" section of #574.

Background

  • There's a a lot of content, both Google-owned and external, that reference "Online Boutique".
  • So the Cymbal rebrand will not display the "Cymbal Shops" wording and logo by default.
  • By default, we display the "Online Boutique" logo and wording across the front-end.
  • Anyone who wants to deploy microservices-demo with the "Cymbal Shops" branding will have to set an environment variable CYMBAL_BRANDING to true (in the frontend microservice), similar to Bank of Anthos.

Change Summary
This pull-request

  • implements the CYMBAL_BRANDING environment variable.
  • documents the use of CYMBAL_BRANDING in docs/cymbal-shops.md.

Additional Notes

  • See my deployment at http://34.74.49.184/ to view the "Cymbal Shop" logo and wording (i.e., CYMBAL_BRANDING is set to "true").

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 24, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 24, 2021
@NimJay NimJay changed the title Support CYMBAL_LOGO env. variable Support CYMBAL_LOGO environment variable Sep 24, 2021
@github-actions
Copy link

🚲 PR staged at http://35.231.157.100

@github-actions
Copy link

🚲 PR staged at http://35.231.157.100

@NimJay NimJay force-pushed the nimjay-cymbal-branding-logo branch 2 times, most recently from a8a05f2 to fb36f5b Compare September 24, 2021 18:21
@github-actions
Copy link

🚲 PR staged at http://35.231.157.100

1 similar comment
@github-actions
Copy link

🚲 PR staged at http://35.231.157.100

Copy link
Member

@Shabirmean Shabirmean left a comment

Choose a reason for hiding this comment

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

Looks good! Some minor thoughts left as comments.

Also have we discussed about running an instance of the repo with the Cymbal Branding as well like the onlineboutique site?

docs/cymbal-shops.md Show resolved Hide resolved
docs/cymbal-shops.md Outdated Show resolved Hide resolved
src/frontend/handlers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@askmeegs askmeegs left a comment

Choose a reason for hiding this comment

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

Tested on my cluster + working. All good, only request is to remove the changes to release/.

@github-actions
Copy link

🚲 PR staged at http://35.231.157.100

Copy link
Member

@Shabirmean Shabirmean left a comment

Choose a reason for hiding this comment

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

LGTM!

@NimJay NimJay changed the title Support CYMBAL_LOGO environment variable Support CYMBAL_BRANDING environment variable Sep 27, 2021
@NimJay
Copy link
Collaborator Author

NimJay commented Sep 27, 2021

All issues raised by reviewers are addressed. Merging.

@NimJay NimJay merged commit 1782837 into master Sep 27, 2021
@NimJay NimJay deleted the nimjay-cymbal-branding-logo branch September 27, 2021 15:02
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
* Support CYMBAL_LOGO env. variable

* Add documentation for CYMBAL_LOGO

* Allow case variation in CYMBAL_LOGO value

* Rename CYMBAL_LOGO to CYMBAL_BRANDING
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants