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

Add optimize to helm charts #287

Merged
merged 16 commits into from
Apr 21, 2022
Merged

Add optimize to helm charts #287

merged 16 commits into from
Apr 21, 2022

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Apr 21, 2022

This PR contains:

  • Add a new sub-chart for optimize (most resources were copied from Tasklist/Operate)
  • Add default values to the values.yaml file
  • Integrate Optimize with Identity (add configuration on both sides)
  • Disable optimize if identity authentication is disabled
  • Add template tests

Manual testing:

After installing we see:

notes

Doing the necessary port-forwards we can login

optimize

Next Steps:

Reviewers:

I know it looks like an big PR, but most of the lines are due to tests and golden files, which you can skip. :)

@oleschoenburg @npepinpe whoever has more time please have a look at the PR
@RomanJRW could you please check the configuration to whether this makes sense in your opinion?

related to #126

Add a new sub-chart for Optimize. Copied resources from tasklist and operate and adjusted to our needs.
Define in camunda-platform values.yaml the default values for Optimize sub-chart.
Add condition for sub-chart such that it is disabled if identity integration is disabled, since optimize has a dependency on identity.
Previous name (zeebe) was misleading
value: {{ .Values.global.identity.auth.publicIssuerUrl | quote }}
- name: CAMUNDA_OPTIMIZE_IDENTITY_ISSUER_BACKEND_URL
value: "http://{{ include "common.names.dependency.fullname" (dict "chartName" "keycloak" "chartValues" . "context" $) | trunc 20 | trimSuffix "-" }}:80/auth/realms/camunda-platform"
- name: CAMUNDA_OPTIMIZE_IDENTITY_CLIENTID
Copy link
Member Author

Choose a reason for hiding this comment

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

@RomanJRW it seems that Optimize only supports CLIENTID the other tools, like tasklist and operate use (or support) CLIENT_ID Is this expected? I stumbled over it, because it didn't worked without the right settings here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads up. Once JIRA wakes up, I'll create a ticket for us to change this in future 👍

Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

🚀

  • 🤡 Kind of weird that we can't use a single .helmignore for all the charts together 🤷‍♂️
  • At this point, I feel like the secret helpers might be, uh, abstracted? Is that even possible without making it too confusing (I mean, this is templated YAML, so it's already confusing 😄)
  • ❓ I couldn't find where we specify the number of Zeebe partitions in the Optimize deployment - isn't that required? 🤔

The main blocker is that some golden file tests (for deployment for example) are pointing to the Operate chart, so the optimize deployment is not tested 😄

Co-authored-by: Nicolas Pepin-Perreault <43373+npepinpe@users.noreply.github.com>
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

I think you'll need to regenerate the golden files, but after that, no blockers from side 👍

@Zelldon
Copy link
Member Author

Zelldon commented Apr 21, 2022

Thanks for your quick review @npepinpe

Regarding your comments:

clown_face Kind of weird that we can't use a single .helmignore for all the charts together man_shrugging

Yeah I can check this.

At this point, I feel like the secret helpers might be, uh, abstracted? Is that even possible without making it too confusing (I mean, this is templated YAML, so it's already confusing smile)

Yeah agree but tbh I don't want to rewrite the logic of bitnami, it is in general quite cool to use their helpers here. I could try, if I find the time, to add some abstractions around it.

question I couldn't find where we specify the number of Zeebe partitions in the Optimize deployment - isn't that required? thinking

As I wrote above this needs to be fixed with #286 and related discussion here https://camunda.slack.com/archives/C02UT3T8DQR/p1650536952860079 Please put your comments or opinions here if you have any :)

This test is tested in the global-deployment
Copy link
Contributor

@RomanJRW RomanJRW left a comment

Choose a reason for hiding this comment

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

Looking mostly at config this seems fine. Some minor comments and one probable missing config for the logout, but otherwise can't see any issue 👍

charts/camunda-platform/test/global_deployment_test.go Outdated Show resolved Hide resolved
value: {{ .Values.global.identity.auth.publicIssuerUrl | quote }}
- name: CAMUNDA_OPTIMIZE_IDENTITY_ISSUER_BACKEND_URL
value: "http://{{ include "common.names.dependency.fullname" (dict "chartName" "keycloak" "chartValues" . "context" $) | trunc 20 | trimSuffix "-" }}:80/auth/realms/camunda-platform"
- name: CAMUNDA_OPTIMIZE_IDENTITY_CLIENTID
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads up. Once JIRA wakes up, I'll create a ticket for us to change this in future 👍

@Zelldon
Copy link
Member Author

Zelldon commented Apr 21, 2022

Thanks @npepinpe and @RomanJRW for your time and review 👍 I applied all review hints and will now happily merge this :)

@Zelldon Zelldon merged commit 2d16ddb into main Apr 21, 2022
@Zelldon Zelldon deleted the zell-add-optimize branch April 21, 2022 21:32
@Zelldon Zelldon mentioned this pull request Apr 25, 2022
19 tasks
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.

None yet

3 participants