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

Create Helm chart GitHub action #224

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

justyntemme
Copy link
Contributor

This adds the GitHub action to automatically create a helm chart on push to main. I will be testing the generated helm chart locally, and adding the custom configuration it may need for any block storage, open ports, or custom dns information among other changes to the default helm chart.

@justyntemme justyntemme mentioned this pull request Mar 2, 2023
@DavidNix
Copy link
Contributor

DavidNix commented Mar 3, 2023

Thanks for this PR! I’m at a conference through tomorrow. I will take a look at this PR on Monday. Sorry for the delay.

@DavidNix
Copy link
Contributor

DavidNix commented Mar 6, 2023

Thanks for the additions. I think we need the helm chart first before merging this PR. Per https://github.com/helm/chart-releaser-action I like and have previously used the convention of /charts directory.

Additionally, I feel this needs to be a separate job from build-and-push-image perhaps release-helm-chart or similar.

@justyntemme
Copy link
Contributor Author

@DavidNix Thanks for the review, I will work to have that deployed into /charts with the name 'cosmos-operator'. Will test to a local Kubernetes repo and ensure things are starting.

@justyntemme
Copy link
Contributor Author

I have created the charts in the directory /charts, however they still need testing before adding the directory to the releaser action config file. Is there a Strangelove dev environment that can be used? I can deploy to my microk8s cluster at home, but I don't want to be solving platform bugs that wouldn't happen in gcp, as making it agnostic can happen in a further PR.

@DavidNix
Copy link
Contributor

Is there a Strangelove dev environment that can be used?

There is but we have competing priorities at the moment, so we won't be able to set up such an environment for a few sprints. I think microk8s or similar may be the best bet for now.

I'll endeavor to look over this PR by EOW. Thank you!

Fwiw, for the future, I'm hoping there's a pattern where we can use the output of kustomize build to auto populate this helm chart so we don't have to keep the helm chart and kustomize files in sync. At SL, we deploy just fine using kustomize only.

@justyntemme
Copy link
Contributor Author

Fwiw, for the future, I'm hoping there's a pattern where we can use the output of kustomize build to auto populate this helm chart so we don't have to keep the helm chart and kustomize files in sync. At SL, we deploy just fine using kustomize only.

Yes I can make that a part of this PR. I did it by hand but can add a pipeline step that passes the output of kustomize to helmify, then we should be able to use those charts as the basis for the release action. Will have that done today, but no rush on your end, I understand things may take priority over QOL enhancements, but hope to make development easier for you and the team :)

@justyntemme
Copy link
Contributor Author

Pushing up the latest changes, we currently remove the old cosmos-operator helm chart directory before moving the generating one to ./charts, then pushing directly to main. Do we prefer this have a pull request? if not we may have to allow githubs account to push to protected branches if we still want the feature that every time we change kustomize that it auto generates a new helm chart on merge. We could only have the helm charts as a release object, and not in the repo, however I think that may be confusing for some devs

@DavidNix
Copy link
Contributor

Pushing up the latest changes, we currently remove the old cosmos-operator helm chart directory before moving the generating one to ./charts, then pushing directly to main. Do we prefer this have a pull request? if not we may have to allow githubs account to push to protected branches if we still want the feature that every time we change kustomize that it auto generates a new helm chart on merge. We could only have the helm charts as a release object, and not in the repo, however I think that may be confusing for some devs

Apologies for my belated response. Last week was unusually busy.

A couple issues:

  1. We do not want automation committing and pushing to upstream. This is dangerous from a security standpoint. Instead, opening a PR for a human to review is good. Or, in this case, adding a make action which updates the helm chart and a human commits + opens PR. The make action path may be quickest.
  2. A +9,800 PR is extremely large and difficult to review. Some of that is due to the large amount of yaml needed. But there may be areas we can make this PR more manageable.

After some thought, my suggestion is to remove the CICD in this PR and work on just the helm chart itself. We can manually test the helm chart generation + deployment in a test cluster. We add back the CICD workflow in a later PR.

@DavidNix
Copy link
Contributor

We may not be able to 100% make a helm chart from kustomize build given the user will want to customize attributes in a values.yaml.

Comment on lines +3 to +7
on:
pull_request:
types:
- closed

Copy link
Contributor

Choose a reason for hiding this comment

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

This workflow file looks great. But we actually only want to release on tags. Also, as I mentioned in a comment, I recommend pulling the work into a separate PR.

# 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: "0.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, apiVersion matches the latest git tag which corresponds to the latest operator release.

@justyntemme
Copy link
Contributor Author

Pushing up the latest changes, we currently remove the old cosmos-operator helm chart directory before moving the generating one to ./charts, then pushing directly to main. Do we prefer this have a pull request? if not we may have to allow githubs account to push to protected branches if we still want the feature that every time we change kustomize that it auto generates a new helm chart on merge. We could only have the helm charts as a release object, and not in the repo, however I think that may be confusing for some devs

Apologies for my belated response. Last week was unusually busy.

A couple issues:

  1. We do not want automation committing and pushing to upstream. This is dangerous from a security standpoint. Instead, opening a PR for a human to review is good. Or, in this case, adding a make action which updates the helm chart and a human commits + opens PR. The make action path may be quickest.
  2. A +9,800 PR is extremely large and difficult to review. Some of that is due to the large amount of yaml needed. But there may be areas we can make this PR more manageable.

After some thought, my suggestion is to remove the CICD in this PR and work on just the helm chart itself. We can manually test the helm chart generation + deployment in a test cluster. We add back the CICD workflow in a later PR.

I think I agree with everything you stated, and appreciate your input. The PR route was my preferred method as well, but didn't want to change until I had your approval. Will look to break these out these out tonight as I have time. Definitely a 2 part PR. I have tested the helm deployment, tho not with everything configured, and can confidently say it deploys, however I think some further testing is in order. :D

@DavidNix
Copy link
Contributor

Thank you, I appreciate your understanding here and your effort!

FWIW, at Strangelove, we use the kustomize present in the config directory and Flux2 to continuously deploy. It works well and circumvents helm's inability to upgrade CRDs.

@justyntemme
Copy link
Contributor Author

Apologies on the delay, this has not been abandoned I've just been sick recently, should wrap this up today & tomorrow.

@tmccoy14
Copy link

tmccoy14 commented Jan 19, 2024

@justyntemme @DavidNix Is there an update on this PR by chance? I was looking into the Helm chart route for deploying the operator. I'd be happy to help test out if needed. Thanks!

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.

3 participants