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

Added helm values, templates, and readme #963

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

miles-w-3
Copy link
Contributor

Closes #960 by adding structure to the helm values.yaml. Added functionality to simplify linking to an external postgres module using a helm template. This creates a pattern that can be used to simplify other processes, like using a PVC for external storage.

I would be happy to iterate on the helm values format or add additional functionality if there is interest. I also think that this should be merged after PR #954, and I can rebase the branch off of those changes once approved. This allows us to fully leverage the unified functionality of $.Release.Namespace

@shanemcd
Copy link
Member

@shanemcd
Copy link
Member

Running CI checks against #954 now and enabled auto-merge on that PR.

@miles-w-3
Copy link
Contributor Author

Cool, I will rebase against those changes and make sure all of my changes are compatible once that's been merged in.

@miles-w-3 miles-w-3 force-pushed the helm-values branch 2 times, most recently from 438dd7a to 970c513 Compare June 26, 2022 20:10
@miles-w-3
Copy link
Contributor Author

@shanemcd I believe I've fixed the newlines, going to take a look at the #954 pipeline to see why it's failing

@miles-w-3
Copy link
Contributor Author

Added updates to the starter readme as well as the main readme. Rebased onto @bewing's branch that includes the update -n awx to the workflwo

@miles-w-3 miles-w-3 force-pushed the helm-values branch 2 times, most recently from e3d543e to b9f2730 Compare June 26, 2022 22:55
@@ -253,6 +253,8 @@ For an example using the Nginx Controller in Minukube, don't miss our [demo vide

For those that wish to use [Helm](https://helm.sh/) to install the awx-operator to an existing K8s cluster:

The helm chart is generated from the `helm-chart` Makefile section using the starter files in `.helm/starter`. Consult [the documentation](.helm/starter/README.md) on how to customize the AWX resource with your own values.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@shanemcd
Copy link
Member

The test output is hard to read, but search the raw output for "molecule test -s kind" and you'll see yamllint is failing. I can't even tell what file it's complaining about. Disabling colored output might help. If not, maybe try running it locally. 🤷

@miles-w-3
Copy link
Contributor Author

I'll take a look, also if #954's workflow works this time I can base my branch off of it directly to make sure it's as accurate as possible

@miles-w-3
Copy link
Contributor Author

miles-w-3 commented Jun 27, 2022

@shanemcd The yaml lint is taking issue with native helm syntax. There are two important pieces of syntax it disagrees with:

  1. Template variables should have spaces between the brackets, as specified here: https://helm.sh/docs/chart_best_practices/templates/#formatting-templates
  2. it doesn't like the - character, but this is really important for controlling the whitespace that templates create, detailed here: https://helm.sh/docs/chart_template_guide/control_structures/#controlling-whitespace

In terms of adding --- to the top of each file, it's not normal in helm, especially in values.yaml, but i dont think it will break anything to do so

Is it possible to ignore the yaml lint on the helm directory? Here is the test output I'm referring to:
image

@miles-w-3 miles-w-3 force-pushed the helm-values branch 5 times, most recently from 676f236 to 5ce9ea3 Compare June 28, 2022 02:35
@miles-w-3
Copy link
Contributor Author

miles-w-3 commented Jun 28, 2022

Ok in my branch I added .helm/starter to the .yamllint ignore config. I think this makes sense because there is already a helm section set up. To compensate, I added some new validation steps to the helm validation section:

  1. Run helm lint against the config - this provides some limited checks, especially to syntax and will probably have more descriptive error messages
  2. Run kubeval: This is a much more rigorous set of tests designed to make sure that kubernetes manifests follow the given schema - Secret, ClusterRole, etc. To use with helm, i create an output of all of the helm templates using helm template. This has the added benefit of debugging the schemas. Kubeval also has special integration with the templated file, it will use the comments to trace the error back to the template that is created. Here is the link to the kubeval site

I hope this is enough testing and makes up for removing the other molecule tests - I think it makes sense to break them up fully because they are different uses of yaml. Let me know if you'd like me to add more tests. Also, here is the output of these tests in a mirror PR on my downstream fork

@janorn
Copy link
Contributor

janorn commented Jun 28, 2022

Perhaps this could be useful to validate the chart?

sbaudoin/yamllint#16 (comment)

@miles-w-3
Copy link
Contributor Author

Perhaps this could be useful to validate the chart?

sbaudoin/yamllint#16 (comment)

Yeah, we could also run yamllint against the template file, but I would argue that kubeval accomplishes and extends that functionality. The lines I have added look very similar to what you linked:

helm template -n awx awx-operator > tmp/test.yaml
kubeval --strict --force-color --ignore-missing-schemas tmp/test.yaml

@miles-w-3
Copy link
Contributor Author

Cool, tests passed. @shanemcd would you be willing to merge the create namespace PR branch?

@shanemcd
Copy link
Member

@miles-w-3 Apologies for the delay, I had to prioritize some other stuff for a while. I've merged that PR. Once you fix the conflicts here we'll get this one in. Thanks for all your work.

@miles-w-3
Copy link
Contributor Author

No problem, rebasing off of new devel now and will update the PR

@miles-w-3
Copy link
Contributor Author

miles-w-3 commented Jul 12, 2022

@shanemcd rebased off of devel, passed tests on my fork so should be ready to go.
Out of curiosity, how often do you do releases, and how often do you push the packaged helm chart to the image host?
Also, if you have more features you'd like to see in the helm chart I would be happy to help out, I'm thinking of doing an ingress template similar to postgres next

@shanemcd
Copy link
Member

Out of curiosity, how often do you do releases, and how often do you push the packaged helm chart to the image host?

Roughly every 3 weeks but we dont have a schedule set in stone as sometimes we have to prioritize working on stuff for customers. A release went out just now but we can do another one ad-hoc for this if it would help you.

The helm chart gets automatically published during the release process:

- name: Release Helm chart
run: |
ansible-playbook ansible/helm-release.yml -v \
-e operator_image=quay.io/${{ github.repository }} \
-e chart_owner=${{ github.repository_owner }} \
-e tag=${{ github.event.release.tag_name }} \
-e gh_token=${{ secrets.GITHUB_TOKEN }}

@shanemcd shanemcd merged commit 5f06e90 into ansible:devel Jul 12, 2022
@miles-w-3
Copy link
Contributor Author

That would be great if you wouldn't mind, these changes would definitely be useful for me to implement/document for my team. Thanks for the info, I look forward to contributing more in the future

@shanemcd
Copy link
Member

@miles-w-3 Just pushed out 0.25.0 that includes your code. Thanks again!

@nickjmv
Copy link

nickjmv commented Sep 5, 2022

@miles-w-3, any idea when you might have time to create that ingress template you were talking about? ;)

... Also, if you have more features you'd like to see in the helm chart I would be happy to help out, I'm thinking of doing an ingress template similar to postgres next

@miles-w-3
Copy link
Contributor Author

Sure, I could get started on it next week. Any particular specs you're looking for? I'll probably make a pretty standard ingress spec with most of the fields (rules, path, https hosts) overridable

@nickjmv
Copy link

nickjmv commented Sep 5, 2022

Nothing particular, just the basics to give it a custom DNS 😄

@nickjmv
Copy link

nickjmv commented Sep 13, 2022

@miles-w-3, not to push you, but let me know the PR you will create for that ingress, I like to see/follow the code to learn.

@miles-w-3
Copy link
Contributor Author

Starting now, I will link the PR here

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.

Build out helm template values for AWX manifest
4 participants