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

Helmchart revamp #262

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

Lite5h4dow
Copy link

@Lite5h4dow Lite5h4dow commented Sep 26, 2024

Definition of Ready

  • I am happy with the code
  • Short description of the feature/issue is added in the pr description
  • PR is linked to the corresponding user story
  • Acceptance criteria are met
  • All open todos and follow ups are defined in a new ticket and justified
  • Deviations from the acceptance criteria and design are agreed with the PO and documented.
  • No debug or dead code
  • My code has no repetitions
  • Documentation/examples are up-to-date
  • All non-functional requirements are met
  • If possible, the test configuration is adjusted so acceptance tests cover my changes

There are quite a few changes here, so I'm going to list them.

  • Helm chart now has 2 subcharts. PostgreSQL and cockroachdb. Both are optional with a toggle at postgresql.enabled and cockroachdb.enabled they are mutually exclusive and the template with throw a fit if you enable both. (actually that might need moving out of the certjob template....) also, they are both disabled by default, so nothing should change for user deployments if they did it with a separate chart before.
  • Artifact Hub details have been added, like social links, and source links. Readme should now also populate as the repo is listed in the Artifact Hub links.
  • screenshots have been added to the readme
  • the PostgreSQL version in the examples has been updated to the latest version of 14 (i believe that was the minimum required version as of a recent update)
  • added certificate creation as an option instead of it just being an example, now its integrated.

@hifabienne hifabienne added the os-contribution This is a contribution from our open-source community label Sep 26, 2024
@Lite5h4dow Lite5h4dow marked this pull request as ready for review September 26, 2024 19:29
@Lite5h4dow
Copy link
Author

https://github.com/gruntwork-io/terratest/tree/master/examples/helm-dependency-example
https://github.com/gruntwork-io/terratest/blob/master/test/helm_dependency_example_template_test.go

Current build issues are regarding dependancies being missing (makes sense seeing as this is the first time dependancies are even in this). but something seems to be missing in the test to pull those dependancies, gonna scour the tests, see if i cant find it

@Lite5h4dow
Copy link
Author

looks like the tests are expecting every pod declared even if the dependancy is optional 😪

@Lite5h4dow
Copy link
Author

Looks like your missing BuildDependancies from the tests, is that intentional?

@eliobischof
Copy link
Member

@Lite5h4dow we decided to push the idea of using subcharts with the cert job to the backlog for now. There are pros and cons, and we'd like to figure out a good solution.

Is it ok for you to keep the PR open as a draft, so we can pick it up again? I think it will not be too far in the future.

@eliobischof eliobischof marked this pull request as draft October 9, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-contribution This is a contribution from our open-source community
Projects
Status: 📨 Product Backlog
Development

Successfully merging this pull request may close these issues.

3 participants