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

[DO NOT MERGE] add roman chart #2935

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

amitsagtani97
Copy link
Contributor

@amitsagtani97 amitsagtani97 commented Dec 19, 2022

Checklist

  • Added helm chart for Roman service.
  • Added postgres 13 chart as optional external dependency

Example values file for both charts can be found at - wireapp/wire-server-deploy#597

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@amitsagtani97 amitsagtani97 temporarily deployed to cachix December 19, 2022 10:13 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix December 19, 2022 10:13 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 19, 2022
@amitsagtani97 amitsagtani97 temporarily deployed to cachix December 19, 2022 12:35 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix December 19, 2022 12:35 — with GitHub Actions Inactive
charts/external-charts/postgresql-11.9.8/README.md Outdated Show resolved Hide resolved
charts/roman/README.md Outdated Show resolved Hide resolved
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 2, 2023 09:55 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 2, 2023 09:55 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 2, 2023 10:05 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 2, 2023 10:05 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 3, 2023 12:42 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 3, 2023 12:42 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 5, 2023 08:30 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 5, 2023 08:30 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 5, 2023 09:37 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 temporarily deployed to cachix January 5, 2023 09:37 — with GitHub Actions Inactive
@amitsagtani97 amitsagtani97 changed the title add roman and postgres 13 chart [DO NOT MERGE] add roman chart Feb 21, 2023
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

I did not test this chart, there may be problems; but glancing at it it looks mostly fine. See comments inline for some suggested improvements.

@@ -0,0 +1 @@
Roman charts are now available.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Roman charts are now available.
A helm chart for the bot 'Roman' is now available.

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version: 0.1.0
version: 0.0.42

@@ -0,0 +1,23 @@
apiVersion: extensions/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

please change the ingress apiVersion for maximum kubernetes version compatibility as done in #3002: add some function to _helpers.tpl and adjust the ingress.yaml in the same fashion as done in that PR for other charts, e.g. the nginx-ingress-services one.

tag: staging
serviceToken: # Provide the roman service token.
# randomly generated for local testing
appKey: b53181dd-6400-4960-8988-f775545588ff-0949f503-421e-4588-a2c5-f64fd9c180fd
Copy link
Member

Choose a reason for hiding this comment

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

I would comment out most of these variables. It's useful for people installing the chart to have an idea of what an example value looks like; but it's not a good idea if people mistakenly use these default values as they forget to override them. It would be better to have the chart fail to install if a variable is missing.

@jschaul
Copy link
Member

jschaul commented Feb 21, 2023

You also need to add this chart to wire-server/Makefile 's list of charts to release (see at the top of the makefile there's a space-separated list of chart names).

@CLAassistant
Copy link

CLAassistant commented May 10, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants