Control the policy controller monitoring resources from chart, enhanc… - #1687
Control the policy controller monitoring resources from chart, enhanc…#1687senanz wants to merge 30 commits into
Conversation
Signed-off-by: Cody Soyland <codysoyland@github.com> Signed-off-by: Senan Zedan (EXT-Nokia) <senan.zedan.ext@nokia.com>
…store#1683) Bumps [google-github-actions/auth](https://github.com/google-github-actions/auth) from 2.1.6 to 2.1.7. - [Release notes](https://github.com/google-github-actions/auth/releases) - [Changelog](https://github.com/google-github-actions/auth/blob/main/CHANGELOG.md) - [Commits](google-github-actions/auth@8254fb7...6fc4af4) --- updated-dependencies: - dependency-name: google-github-actions/auth dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Senan Zedan (EXT-Nokia) <senan.zedan.ext@nokia.com>
…ment for the current implmentation to hadd all the resources by default, The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart Signed-off-by: Senan Zedan (EXT-Nokia) <senan.zedan.ext@nokia.com>
|
@vaikas - Could you please look into this? |
|
Could you add some tests, maybe here: https://github.com/sigstore/policy-controller/tree/main/test |
|
@vaikas - per your request, test added. |
That does not really test any of this new code. You'd have to add a test that launches policy controller with these new flags, here's one example where we change the policy-controller behaviour by the flags: So, create a new kustomize file that launches policy controller with say --resource-name=pods, and customize it like here: And then after the policy-controller has been started with the flags under test, you'd run your new test: It should at the bare minimum have a negative test and a positive test. So, if you're using resource-name pods, then launching a deployment with a failing config should succeed if you instead launch a pod. |
vaikas
left a comment
vaikas
left a comment
There was a problem hiding this comment.
Looking great! There's just some nits for naming, namespaces. One change that should be made it so make sure the tag->digest resolution does not actually happen. The webhook configuration is a good check, but it's easy enough to also make sure the actual resource (deployment) does not get modified.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1687 +/- ##
===========================================
- Coverage 52.92% 42.66% -10.27%
===========================================
Files 44 121 +77
Lines 3979 9010 +5031
===========================================
+ Hits 2106 3844 +1738
- Misses 1651 4811 +3160
- Partials 222 355 +133 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Looks like some of the changes I requested, and you marked as fixed have not made it in to this PR 🤔 ? Also, looks like bunch of |
|
Hi Ville,
Fixed them locally and working in the last comment to add tests.
Updated PR will raise soon.
…On Mon, 9 Dec 2024 at 18:48 Ville Aikas ***@***.***> wrote:
Looks like some of the changes I requested, and you marked as fixed have
not made it in to this PR 🤔 ?
Also, looks like bunch of sed commands are failing, for example:
https://github.com/sigstore/policy-controller/actions/runs/12234409121/job/34140387122?pr=1687#step:14:132
—
Reply to this email directly, view it on GitHub
<#1687 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIDIUC3MMEW4UY6HLKCMUCL2EXCUBAVCNFSM6AAAAABRCWYFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRYGY3TMOBYGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
apolofize for the delay, updated. |
…ment for the current implmentation to hadd all the resources by default, The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart Signed-off-by: Senan Zedan (EXT-Nokia) <senan.zedan.ext@nokia.com>
…ller into issues_1388 # Conflicts: # cmd/webhook/main.go
| - name: Run Custom resources Tests | ||
| timeout-minutes: 5 | ||
| run: | | ||
| chmod +x ./test/e2e_test_policy_custom_resource.sh |
There was a problem hiding this comment.
FYI, you should be able to make this change in git so that you don't have to do this here.
|
@senanz Thank you for creating a great PR. I'm looking forward to this PR being merged and released soon. Do you have any plans to update this PR? If for any reason it is difficult to update the PR, I'm considering offering my assistance. |
|
hi @0xiso , Will back to work on that early next week, will ping you on slack and for sure we can collaborate. |
|
Hi @senanz, I apologize for asking multiple times, but are there any updates? Please let me know if there's anything I can do to help merge this PR; I'd be happy to. |
|
Hi @senanz , sorry for bothering but do you still plan to give this a go? |
| @@ -0,0 +1,149 @@ | |||
| # Copyright 2022 The Sigstore Authors. | |||
| @@ -0,0 +1,114 @@ | |||
| #!/usr/bin/env bash | |||
| # | |||
| # Copyright 2024 The Sigstore Authors. | |||
6 participants
…ment for the current implmentation to hadd all the resources by default, The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart
This PR resolves #1388
Signed-off-by: Senan Zedan (EXT-Nokia) senan.zedan.ext@nokia.com
Summary
The new change add avail to pass resourcesNames through the chart with list of resources comma sperataed for which resources to be monitored by the policy controller, the default is all resources if the flag wasn't presented in the chart