Skip to content
This repository has been archived by the owner on Nov 7, 2019. It is now read-only.

Add helm chart #113

Merged
merged 1 commit into from
Nov 2, 2018
Merged

Add helm chart #113

merged 1 commit into from
Nov 2, 2018

Conversation

acaire
Copy link
Contributor

@acaire acaire commented Oct 10, 2018

Attempt to address #82

Description of changes:
Adds a Helm chart for aws-service-operator and correct a minor typo with some perceived brand consistency. Happy to drop the last commit if necessary - I haven't tested it heavily, feel free to change anything you need to :)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@acaire
Copy link
Contributor Author

acaire commented Oct 10, 2018

Just noticed i'm missing NOTES.txt - Will work on this tomorrow, until then feedback appreciated, thanks!

@KierranM
Copy link

👍 Had just started making this helm chart myself in the helm/charts repo before I seen this. Quick question, how/where will consumers(me in this case) of the chart access it from? One of the main advantages of putting charts in the helm/charts repo is that you get that for free. Do y'all have your own helm repo?

Copy link
Contributor

@christopherhein christopherhein left a comment

Choose a reason for hiding this comment

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

Thank you @acaire this is fantastic. Couple changes.

Also, you squash them all into a single commit when done?

Really appreciate you contributing this back!

charts/aws-service-operator/README.md Outdated Show resolved Hide resolved
charts/aws-service-operator/templates/deployment.yaml Outdated Show resolved Hide resolved
@christopherhein
Copy link
Contributor

👍 Had just started making this helm chart myself in the helm/charts repo before I seen this. Quick question, how/where will consumers(me in this case) of the chart access it from? One of the main advantages of putting charts in the helm/charts repo is that you get that for free. Do y'all have your own helm repo?

I think we should copy it over once we have this stable, long-term there should be a release mechanism built into a CI process having the source in the repo will help as features like #107 get finalized which drastically change the way the CRD manifests are handled.

@tantona tantona assigned tantona and unassigned tantona Oct 15, 2018
Copy link

@davidxjohnson davidxjohnson left a comment

Choose a reason for hiding this comment

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

Should all the kind: CustomResourceDefinition manifests be included in this helm chart?

@christopherhein
Copy link
Contributor

They should be now @davidxjohnson and @acaire, the latest release removed operator-kit which was managing the CRDs now we have them config/aws-service-operator.yaml

@davidxjohnson
Copy link

I was anticipating the files in examples/cloudformationtemplates to be included in the chart since they are needed by the aws-operator.

@christopherhein
Copy link
Contributor

@davidxjohnson by default they actually aren’t required, there is a fallback mechanism to an S3 bucket which holds the canonical version of the assets and will try to load from that bucket after checking the user provided bucket.

@davidxjohnson
Copy link

I see that the CRD manifests are in configs/aws-service-operator.yaml ... but wondering if they should be in charts/aws-service-operator/ ??? ... didn't see them in the list of file changes.

@christopherhein
Copy link
Contributor

Yes @davidxjohnson those will need to be moved into the charts as well. They weren't included because when this was first authored the CRDs were deployed by the go application which they aren't now. See #107 for more details

@davidxjohnson
Copy link

I'll wait for the helm chart work to merge and will look at it again then.

Regards

@acaire
Copy link
Contributor Author

acaire commented Oct 24, 2018

@christopherhein Added these manually, is it worth trying to do it programatically i.e. create a separate templ file to be generated inside the charts/template directory?

@christopherhein
Copy link
Contributor

We can do the code generation outside of this PR.

@acaire
Copy link
Contributor Author

acaire commented Nov 2, 2018

@christopherhein I've made these changes - Can you please let me know if anything further is required?

@christopherhein christopherhein added lgtm PR is ready to be merged approved PRs that are approved labels Nov 2, 2018
@christopherhein christopherhein merged commit d1d2eef into amazon-archives:master Nov 2, 2018
@christopherhein
Copy link
Contributor

Thanks @acaire really appreciate this work! We're all merged, I'll be cutting a new build in the coming week and we'll have a tar.gz that you should be able to use to helm install URL from. For now anyone that finds this PR you can clone the repo and helm install charts/aws-service-operator

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved PRs that are approved lgtm PR is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants