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

move helm chart to reactiveops/charts #128

Merged
merged 2 commits into from
Jun 13, 2019
Merged

move helm chart to reactiveops/charts #128

merged 2 commits into from
Jun 13, 2019

Conversation

rbren
Copy link
Contributor

@rbren rbren commented May 23, 2019

This makes releases a pain. Definitely open to suggestions on how to improve that.

Currently it's three separate commits:

  • Change version in main.go and README, commit to master
  • Tag branch to create GH release and image in quay
  • Update helm chart versions, commit to master in charts repo
  • Update deploy configs using make helm-to-k8s, commit to master

@rbren rbren requested a review from robscott as a code owner May 23, 2019 19:43
@rbren rbren force-pushed the rb/move-helm-chart branch 3 times, most recently from f023638 to d48b6b3 Compare May 28, 2019 12:45
@rbren
Copy link
Contributor Author

rbren commented May 28, 2019

Corey/Andy -

This removes the Helm chart from this repo now that FairwindsOps/charts#82 is merged.

The release process is pretty hairy right now, would love any suggestions. The basic flow is:

  • change all version number references in this repo and merge via PR
  • tag the commit and push
  • update version number in charts repo and merge via PR
  • update deploy scripts in this repo using make helm-to-k8s

The big problem with this is that the deploy YAML files on the branch tagged 0.1.4 will really be for 0.1.3. But we need to get the 0.1.4 branch merged before we can update the helm chart, and we need the new helm chart to update the YAML files.

Is this a problem we've run into in other repos?

@endzyme endzyme self-requested a review June 3, 2019 17:31
@endzyme
Copy link
Contributor

endzyme commented Jun 8, 2019

I'd love to get more insight, in person or whenever you have time, about the ./deploy/ directory value. Based on "Getting Started" and the "Deploy" sections of the README, I am seeing that it's used as simplified instructions to get a user bootstrapped without needing help or other dependencies.

It feels like this type of resource/object is more of a build artifact than something that should constantly be checked in and manually updated. Having the helm chart live in another repository adds some complexity to this, since any changes to the app/config that also affect the helm chart, would need a lockstep release.

How would this sound as an alternative workflow for when we need to update these files:

  1. When we release a version, the pipeline runs the make helm-to-k8s with the context of the GIT_TAG
  2. The pipeline uploads dashboard.k8s.yml and webhook.k8s.yml to the releases page
  3. The README.md references kubectl apply -f https://github.com/reactiveops/polaris/releases/download/<LATESTVERSION>/dashboard.k8s.yml in the getting started instructions

This would allow more automation by having the deploy "user feature" yamls be a build artifact instead of a checked in object in git. It would also avoid forgetting to update the README on every version bump; especially if we included instructions such as:

LATEST_RELEASE=$(curl -L -s -H 'Accept: application/json' https://github.com/reactiveops/polaris/releases/latest)
LATEST_VERSION=$(echo $LATEST_RELEASE | sed -e 's/.*"tag_name":"\([^"]*\)".*/\1/')
kubectl apply -f "https://github.com/reactiveops/polaris/releases/download/$LATEST_VERSION/dashboard.k8s.yml"

Thoughts welcome and feel free to connect if it would be easier to hash out some of my missing context via video or phone :)

@rbren
Copy link
Contributor Author

rbren commented Jun 10, 2019

Thanks Nick! This exactly the kind of insight I was looking for.

Definitely like the idea of moving the deploy config to the releases page. I think you can always refer to the latest release, per here

I'll rework with your suggestions. Thanks!

@rbren rbren requested a review from kimschles as a code owner June 10, 2019 16:26
@rbren rbren force-pushed the rb/move-helm-chart branch 2 times, most recently from 8ccbe19 to 8e050ea Compare June 10, 2019 17:10
@rbren
Copy link
Contributor Author

rbren commented Jun 10, 2019

@endzyme I think the process is much better now thanks to your input. Check out the instructions in CONTRIBUTING.md and let me know what you think.

Still not simple, but definitely more safe/consistent

@coreypobrien
Copy link
Contributor

I generally like the pattern here. I think the only concern I have is that we have to create a chart PR for every release. It is probably something we can address in a future PR, though, by publishing a major-version docker image tag and keeping the helm chart set to a major version tag as well.

init helm in circleci

fix template command
@rbren rbren force-pushed the rb/move-helm-chart branch 3 times, most recently from 2d98321 to fb26503 Compare June 13, 2019 18:10
@rbren
Copy link
Contributor Author

rbren commented Jun 13, 2019

That's a really good point. I've adjusted the instructions so that minor releases only need to change this repo. I'm going to cut a release once this is in - I'll change the chart to use major versions as part of that.

I also added rok8s-scripts to help with the workflow here.

if [[ -z $CIRCLE_PR_NUMBER ]]; then
docker login quay.io -u="reactiveops+circleci" -p="${quay_token}"
docker push $REPO:dev-$CIRCLE_SHA1
docker-push -f .circleci/build.config
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

- setup_remote_docker
- *set_environment_variables
- *docker_build_and_push
- *release_deploy_configs
Copy link
Contributor

Choose a reason for hiding this comment

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

this is great!

@rbren rbren merged commit 50d32b9 into master Jun 13, 2019
@endzyme endzyme deleted the rb/move-helm-chart branch July 3, 2019 15:54
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