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

feat: settings to skip init and ephemeral containers. #74

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Dec 14, 2023

Description

Adds two settings for the policy that allows the user to skip the verification on init and ephemeral containers.
Furthermore, refactor the policy unit tests to use rstest reducing the number of test functions and duplicate code

Fix #73

Test

make test

@jvanz jvanz self-assigned this Dec 14, 2023
@jvanz jvanz requested a review from a team as a code owner December 14, 2023 02:49
@jvanz jvanz added the kind/enhancement New feature or request label Dec 14, 2023
Copy link
Contributor

@kravciak kravciak left a comment

Choose a reason for hiding this comment

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

Could you add questions-ui.yml please?

questions:
- default: false
  tooltip: >-
    Ignore that some init container is configured as privileged
  group: Settings
  label: Skip init containers
  required: false
  type: boolean
  variable: skip_init_containers
- default: false
  tooltip: >-
    Ignore that some ephemeral container is configured as privileged
  group: Settings
  label: Skip ephemeral containers
  required: false
  type: boolean
  variable: skip_ephemeral_containers

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Adds two settings for the policy that allows the user to skip the
verification on init and ephemeral containers.

Signed-off-by: José Guilherme Vanz <[email protected]>
@jvanz
Copy link
Member Author

jvanz commented Dec 14, 2023

@kravciak I've added the questions-ui.yml. Thanks for noticing that! :)

@jvanz jvanz requested review from kravciak and a team December 14, 2023 11:51
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM

src/lib.rs Outdated Show resolved Hide resolved
@fabriziosestito
Copy link
Contributor

Maybe it's redundant, but could we add a couple of e2e tests regarding the new feature?

@jvanz jvanz force-pushed the skip-init-containers branch 2 times, most recently from 08d556b to 4c1ec4f Compare December 15, 2023 19:22
@jvanz
Copy link
Member Author

jvanz commented Dec 15, 2023

Maybe it's redundant, but could we add a couple of e2e tests regarding the new feature?

Done. I've also remove bats and start to run the e2e tests in rust.

@jvanz
Copy link
Member Author

jvanz commented Dec 15, 2023

To get a green CI in the PR we need to merge this PR and update the reusable workflow.

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

@jvanz I think there was a misunderstanding. The existing bats test had to be extended, not rewritten in Rust.

Moreover, the tests are failing inside of the GH runner not because of something missing inside of the shared github actions (this PR you created), but because the policy.wasm file is not found.
The Makefile target running the tests has been changed by this PR to remove the dependency against the annotated-policy.wasm file, that leads to a situation where the e2e tests cannot find the policy.wasm file

Can you please undo the changes to the e2e tests and just extend the bats file?

@jvanz
Copy link
Member Author

jvanz commented Dec 18, 2023

@jvanz I think there was a misunderstanding. The existing bats test had to be extended, not rewritten in Rust.

Moreover, the tests are failing inside of the GH runner not because of something missing inside of the shared github actions (this PR you created), but because the policy.wasm file is not found. The Makefile target running the tests has been changed by this PR to remove the dependency against the annotated-policy.wasm file, that leads to a situation where the e2e tests cannot find the policy.wasm file

This is not the case. The workflow does not call Makefile targets. It call cargo test. I believe the missing file is the kwctl binary.

Can you please undo the changes to the e2e tests and just extend the bats file?

Yes, I can do it. I was just thinking that running the tests on Rust will be more flexible. As we have done for another rust repositories.

Just one additional note, if we had choose the run the tests on Rust, despite of the makefile changes, the other PR will be still required. Because the command called in the workflow to run the tests is cargo test which runs everything. As the e2e uses kwctl, that will failed due the missing kwctl binary (the test workflow does not install it). That's why I've open that PR to run lib tests only. ;-)

@jvanz jvanz force-pushed the skip-init-containers branch 3 times, most recently from 308f2b5 to 64b5803 Compare December 18, 2023 13:01
@jvanz
Copy link
Member Author

jvanz commented Dec 18, 2023

I've just added a little piece of code to validate if the policy.wasm exists before returning the path to it. The tests are still failing with a different error. Thus, I think the problem is still the missing kwctl binary. Therefore, I believe the comments in this PR and in the github action repo still hold. However, this will not make any difference. I'll be reverting the changes to go back to bats. I was just double checking if my hypothesis makes some sense. ;-)

Adds the questions-ui.yml file that allows Rancher UI to show the
possible configurations in the web interface.

Signed-off-by: José Guilherme Vanz <[email protected]>
Refactor the policy unit tests to use rstest reducing the number of test
functions and similar code. Furthermore, added two more e2e tests to validate
the new feature to skip init and ephemeral containers.

Signed-off-by: José Guilherme Vanz <[email protected]>
@jvanz jvanz requested a review from flavio December 19, 2023 14:02
@jvanz jvanz merged commit 7fc38ec into kubewarden:main Dec 19, 2023
6 checks passed
@jvanz jvanz deleted the skip-init-containers branch December 19, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: skip init containers
5 participants