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

Online Boutique's Helm chart #1353

Merged
merged 19 commits into from Dec 8, 2022
Merged

Online Boutique's Helm chart #1353

merged 19 commits into from Dec 8, 2022

Conversation

mathieu-benoit
Copy link
Contributor

@mathieu-benoit mathieu-benoit commented Dec 2, 2022

Online Boutique's Helm chart - #1319.

You can render the hydrated/generated manifests of this Helm chart from its associated folder:

cd helm-chart
helm template .

Or via the manually pushed remote Helm chart:

helm template oci://us-docker.pkg.dev/online-boutique-ci/charts/onlineboutique

You can do the same with both for installing it in your cluster:

cd helm-chart
helm upgrade onlineboutique . --install

Or:

helm upgrade onlineboutique oci://us-docker.pkg.dev/online-boutique-ci/charts/onlineboutique --install

Action items:

  • Create the Helm chart Online Boutique
  • Manually push the Helm chart in Artifact Registry
    • cd helm-chart && helm package . && helm push onlineboutique-0.4.2.tgz oci://us-docker.pkg.dev/online-boutique-ci/charts
  • Tests
  • Reviews
  • Script to push the Helm chart in GAR in the release process

@mathieu-benoit mathieu-benoit requested a review from a team as a code owner December 2, 2022 21:27
@mathieu-benoit mathieu-benoit marked this pull request as draft December 2, 2022 21:27
@mathieu-benoit mathieu-benoit self-assigned this Dec 2, 2022
@mathieu-benoit mathieu-benoit added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 2, 2022
@mathieu-benoit mathieu-benoit linked an issue Dec 2, 2022 that may be closed by this pull request
4 tasks
@github-actions
Copy link

github-actions bot commented Dec 2, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 5, 2022

🚲 PR staged at http://35.188.161.244

@mathieu-benoit mathieu-benoit changed the title [WIP] Online Boutique's Helm chart Online Boutique's Helm chart Dec 5, 2022
@mathieu-benoit mathieu-benoit removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 5, 2022
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

🚲 PR staged at http://35.188.161.244

@mathieu-benoit mathieu-benoit marked this pull request as ready for review December 5, 2022 22:05
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "v0.4.2"
Copy link
Collaborator

@NimJay NimJay Dec 6, 2022

Choose a reason for hiding this comment

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

Issue:
Please add automation to /hack/make-release.sh such that:

  1. the versions inside this Chart.yaml file are updated.
  2. the file is git commit-ed.

Ideally, we would simplify the release process (such that the automation is no longer required), but that would require an audit of docs/demos using Online Boutique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

🚲 PR staged at http://35.188.161.244

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

🚲 PR staged at http://35.188.161.244


recommendationservice:
create: true
name: recommendationservice
Copy link
Collaborator

@NimJay NimJay Dec 6, 2022

Choose a reason for hiding this comment

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

Thought:
What are you thoughts on removing support for variables such as shippingservice.name and emailservice.name?
I understand there is a PM that removes the ...service suffix of each microservice when using Online Boutique — but is there really any value to being able to change the name of microservices?
It also sounds like an edge case.
I am mostly trying to:

  1. reduce the amount of clutter inside the Helm charts similar to {{ .Values.shippingservice.name }}.
  2. reduce the total number of configurations in this Helm chart to simplify the user experience.

To me, the value it adds doesn't seem worth the amount of clutter inside the Helm charts.

What do you think? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, building and maintaining an Helm chart is always a compromise of opiniated/hard-coded values versus more variables/parameters/customization. The latter adds more work for the maintainers that's for sure.

In this case, yes because I knew that someone is using Online Boutique this way (change the name of the services), I by design took this into account. I wanted myself do it in my setup/demos in the past, but changing this in the manitests (+ references in other services) was painful, so as the author/maintainer I think that makes sense to have this quick win.

Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

Thanks again for doing this, Mathieu!
I've made some small commits myself to the mathieu-benoit/helm-chart branch to reduce back-and-forth.

Failed Testing

I've tested helm template . It worked. 👍
However, when I ran helm upgrade onlineboutique . --install, I got the following error:

Error: UPGRADE FAILED: failed to create resource: Deployment in version "v1" cannot be handled as a Deployment: json: cannot unmarshal bool into Go struct field EnvVar.spec.template.spec.containers.env.value of type string

I got the same error for helm upgrade onlineboutique oci://us-docker.pkg.dev/online-boutique-ci/charts/onlineboutique --install. 🤔
Did it work for you?

Running helm version gives me:

version.BuildInfo{Version:"v3.9.3", GitCommit:"414ff28d4029ae8c8b05d62aa06c7fe3dee2bc58", GitTreeState:"clean", GoVersion:"go1.17.13"}

I'm doing all this on Cloud Shell with my kubectx set to a GKE cluster.

Other Concerns

I've left some comments.
My main concerns are:

  1. hack/make-release.sh
  2. variables for microservice names (e.g., .Values.cartservice.name)

@mathieu-benoit
Copy link
Contributor Author

Thanks for your review @NimJay, will tackle your request/feedback later.

FYI: I just fixed the issue you faced, us-docker.pkg.dev/online-boutique-ci/charts/onlineboutique is also updated accordingly, sorry about that. That should be good to go now.

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 7, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

Copy link
Collaborator

@NimJay NimJay 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 quick turn-around, @mathieu-benoit!
My tests worked!

  1. I ran: helm upgrade onlineboutique . --install
  2. I saw:
    Release "onlineboutique" has been upgraded. Happy Helming!
    NAME: onlineboutique
    LAST DEPLOYED: Thu Dec  8 14:00:28 2022
    NAMESPACE: default
    STATUS: deployed
    REVISION: 4
    TEST SUITE: None
    NOTES:
    Get the application URL by running these commands:
      NOTE: It may take a few minutes for the LoadBalancer IP to be available.
      You can watch the status of by running 'kubectl get --namespace default svc -w frontend-external'
      export SERVICE_IP=$(kubectl get svc --namespace default frontend-external --template "{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}")
      echo http://$SERVICE_IP
    
  3. I ran the 3 commands from the output above. All 3 worked! 👍
  4. I visited the URL for Online Boutique. Worked!
  5. helm upgrade onlineboutique . --install --set frontend.cymbalBranding=true
  6. I revisited the URL for Online Boutique and saw the Cymbal logo, and successfully checked out my cart.

Screenshot 2022-12-08 at 2 15 30 PM

That UX was amazing! 🤯

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

🚲 PR staged at http://35.188.161.244

@NimJay NimJay merged commit f8e2900 into main Dec 8, 2022
@bourgeoisor bourgeoisor deleted the mathieu-benoit/helm-chart branch April 21, 2023 01:34
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
* Create initial Helm chart

* currency and email

* Complete Helm chart for Online Boutique

* cat helm-template.yaml

* Add warning about Helm being in experimental mode

* Use 2022 for Apache license headers

* Link to values.yaml in helm-chart/README.md

* Fix issue with bool in env var

* Fix issue with bool in env var (take 2/2)

* Review naming convention + add externalRedisTlsOrigination

* Fix CI issues

* Fix CI issue with Sidecar

* Add seccompProfile for more security, disable by default

* Helm chart push in release process

* More elegant and consistent way to automate the Helm chart package/push

* Update NOTES.txt

* Update NOTES.txt

* Update NOTES.txt

Co-authored-by: Nim Jayawardena <nimjay@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate Helm integration
2 participants