Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Add Open Policy Agent chart #8915

Merged
merged 4 commits into from Dec 18, 2018
Merged

Conversation

tsandall
Copy link
Collaborator

@tsandall tsandall commented Oct 31, 2018

Hello 馃憢

This is my first chart. Let me know what should be fixed and what can be improved.

What this PR does / why we need it:

This PR adds a chart to deploy OPA as an admission controller. Hopefully this will be useful for others.

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Variables are documented in the README.md (docs are inlined into the values.yaml, hopefully this is ok!)

/cc @gtaylor

@k8s-ci-robot
Copy link
Contributor

@tsandall: GitHub didn't allow me to request PR reviews from the following users: gtaylor.

Note that only helm members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Hello 馃憢

What this PR does / why we need it:

This PR adds a chart to deploy OPA as an admission controller. Hopefully this will be useful for others.

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Variables are documented in the README.md (docs are inlined into the values.yaml, hopefully this is ok!)

/cc @gtaylor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @tsandall. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 31, 2018
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2018
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2018
Copy link
Collaborator

@gtaylor gtaylor left a comment

Choose a reason for hiding this comment

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

Just two very minor suggestions!

As a very optional pointer, it could be worth considering defining a helper to hold your common labels, per this example. This cuts down on some noise in the resource templates, and also allows your users to quickly fork the Chart to add their org-standard labels.

stable/opa/Chart.yaml Outdated Show resolved Hide resolved
stable/opa/README.md Outdated Show resolved Hide resolved
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). labels Nov 8, 2018
- Set app version to OPA vresion per convention
- Prefer user facing chart name to local in install guide
- Refactor standard labels into helper template

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2018
@tsandall
Copy link
Collaborator Author

tsandall commented Nov 8, 2018

@gtaylor thanks for the feedback. I've added another commit addressing those comments.

Copy link
Collaborator

@gtaylor gtaylor left a comment

Choose a reason for hiding this comment

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

I don't have perms to OK this for CI, but this looks good to me!

@tsandall
Copy link
Collaborator Author

@gtaylor is there someone else who we should ping to look at this? It would be cool if we could get this merged. I know of at least one OPA user that's already rolled their own helm chart.

@gtaylor
Copy link
Collaborator

gtaylor commented Nov 28, 2018

I'll see if I can rouse someone in the #charts room in the Kubernetes Slack org. It looks like the review volume has increased to the point of overwhelming the reviewers.

## Configuration

All configuration settings are contained and described in
[values.yaml](values.yaml).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please document the configuration in a table. See the wordpress chart for an example.

@mattfarina
Copy link
Contributor

/ok-to-test

I'm sorry for the delay in reviewing. We are a bit overwhelmed. Look out for changes coming to reduce that pressure on the maintainers.

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2018
Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 10, 2018
@mattfarina
Copy link
Contributor

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattfarina, tsandall

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Dec 18, 2018
@k8s-ci-robot k8s-ci-robot merged commit 44981ee into helm:master Dec 18, 2018
coreypobrien pushed a commit to FairwindsOps/helm-charts that referenced this pull request Dec 31, 2018
* Add Open Policy Agent chart

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Update chart with example to kick the tires

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Update chart per review feedback

- Set app version to OPA vresion per convention
- Prefer user facing chart name to local in install guide
- Refactor standard labels into helper template

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Add configuration table to README

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
wgiddens pushed a commit to wgiddens/charts that referenced this pull request Jan 18, 2019
* Add Open Policy Agent chart

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Update chart with example to kick the tires

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Update chart per review feedback

- Set app version to OPA vresion per convention
- Prefer user facing chart name to local in install guide
- Refactor standard labels into helper template

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

* Add configuration table to README

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
@alexellis
Copy link

@tsandall are there any plans to create an upstream chart outside of the charts repo? Many projects are moving away from the "stable" repo which is going to be considered deprecated for most projects going forward. open-policy-agent/opa#1035

@tsandall
Copy link
Collaborator Author

tsandall commented Oct 5, 2020

@alexellis no immediate plans, though if someone wants to take this on, I'd be happy to support.

@phisco
Copy link

phisco commented Nov 16, 2020

Hi @tsandall, me and @angelbarrera92 would be interested in helping to bring the helm chart to https://artifacthub.io/ .
In case having a vanilla opa chart is still interesting for the project let us know what would be your requirements/expectations.

@tsandall
Copy link
Collaborator Author

@phisco that would be great. Can we migrate the existing (deprecated) chart as-is or will changes be needed? If you can drive the effort, that would be excellent.

@angelbarrera92
Copy link
Contributor

@phisco that would be great. Can we migrate the existing (deprecated) chart as-is or will changes be needed? If you can drive the effort, that would be excellent.

Cio @tsandall !!!

As @phisco said, we want to contribute helping hosting/automating the OPA helm chart releases. I saw in the following thread/issue and interesting comment from a guy who created a guide to migrate helm charts: https://github.com/helm/charts/issues/21103#issuecomment-715907845

TL;DR: We followed: https://github.com/torstenwalter/helm-chart-hosting

This one uses a completly empty github repository + github actions in place to automatic release everything.

We've created a repository under our organization: https://github.com/sighupio/opa-helm-charts, feel free to take a look.
Now, we have multiple options:

  1. We can transfer the ownership of the repository to your organization + We can PR to the new destination with the needed changes (uris mostly).
  2. Create a new repository from scratch. Then we can PR it.
  3. If you consider that's is not the best option for your chart, we can work in another different approach.
    3.1 Maybe include the helm chart inside the same OPA repository.

Thanks!

@alexellis
Copy link

@AlexsJones don't you use OPA in your stack? Would this be relevant to you too?

Once the dust as settled, it would be great to get an arkade app for OPA too.

@angelbarrera92
Copy link
Contributor

angelbarrera92 commented Nov 16, 2020

@phisco that would be great. Can we migrate the existing (deprecated) chart as-is or will changes be needed? If you can drive the effort, that would be excellent.

Cio @tsandall !!!

As @phisco said, we want to contribute helping hosting/automating the OPA helm chart releases. I saw in the following thread/issue and interesting comment from a guy who created a guide to migrate helm charts: #21103 (comment)

TL;DR: We followed: https://github.com/torstenwalter/helm-chart-hosting

This one uses a completly empty github repository + github actions in place to automatic release everything.

We've created a repository under our organization: https://github.com/sighupio/opa-helm-charts, feel free to take a look.
Now, we have multiple options:

1. We can transfer the ownership of the repository to your organization + We can PR to the new destination with the needed changes (uris mostly).

2. Create a new repository from scratch. Then we can PR it.

3. If you consider that's is not the best option for your chart, we can work in another different approach.
   3.1 Maybe include the helm chart inside the same OPA repository.

Thanks!

We can also think about integrating the same release workflow @ gatekeeper repository into the OPA one.

@tsandall
Copy link
Collaborator Author

@angelbarrera92 would it be possible to host the chart under an existing repo? I think that putting into https://github.com/open-policy-agent/kube-mgmt would make the most sense. If that's possible, let me know and we can setup a GitHub team and add you to it so that you can manage it.

@angelbarrera92
Copy link
Contributor

@angelbarrera92 would it be possible to host the chart under an existing repo? I think that putting into https://github.com/open-policy-agent/kube-mgmt would make the most sense. If that's possible, let me know and we can setup a GitHub team and add you to it so that you can manage it.

Ciao @tsandall! Sure it is possible, but it makes it really hard to maintain the git history of the original helm chart (unless you rewriting the kube-mgmt history).
If the git history is something that is not really relevant at this point, I can work on it without major problems.

Feel free to create that team including me and @phisco!

Thanks!

@tsandall
Copy link
Collaborator Author

In this case, I'm not too worried about the git history. I've made a team, granted rights on the kube-mgmt repo, and added both of you! Thanks for working on this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants