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

Gatekeeper hull integration #2624

Conversation

doflamingo721
Copy link
Contributor

Linked issue: rancher/rancher#41026

@doflamingo721 doflamingo721 force-pushed the gatekeeper-hull-integration branch 5 times, most recently from 177f6fc to 07c8944 Compare May 17, 2023 07:52
@doflamingo721 doflamingo721 marked this pull request as ready for review May 29, 2023 03:55
@doflamingo721 doflamingo721 changed the title WIP Gatekeeper hull integration Gatekeeper hull integration May 29, 2023
@doflamingo721 doflamingo721 force-pushed the gatekeeper-hull-integration branch 2 times, most recently from 01be249 to c895934 Compare May 29, 2023 14:40
Name: "Set .postUpgrade.labelNamespace.image.repository and .postUpgrade.labelNamespace.image.tag",

TemplateOptions: chart.NewTemplateOptions(DefaultReleaseName, DefaultNamespace).
SetValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

combine into one .Set() call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

TemplateOptions: chart.NewTemplateOptions(DefaultReleaseName, DefaultNamespace).
SetValue(
"postInstall.labelNamespace.image.repository", "test-gatekeeper-crd-repo",
).
Copy link
Contributor

Choose a reason for hiding this comment

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

.Set()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


TemplateOptions: chart.NewTemplateOptions(DefaultReleaseName, DefaultNamespace).
Set(
"postInstall.labelNamespace.extraAnnotations", testPodAnnotation,
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be merged

@eliyamlevy
Copy link
Contributor

I think a lot of test cases can be combined into one large one, might be a lot more readable. Also would have less cases to run.

When the PR is done we can reevaluate

@doflamingo721 doflamingo721 force-pushed the gatekeeper-hull-integration branch 4 times, most recently from 6581d9b to b16c55a Compare July 5, 2023 16:06
"validatingWebhookFailurePolicy", "Fail",
),
},
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove commented out code from the PR

),
},

// For .Values.postUpgrade
Copy link
Contributor

@eliyamlevy eliyamlevy Jul 19, 2023

Choose a reason for hiding this comment

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

Remove the comment or fix formatting


{
Name: "Set Values for postUpgrade",
TemplateOptions: chart.NewTemplateOptions(DefaultReleaseName, DefaultNamespace).
Copy link
Contributor

Choose a reason for hiding this comment

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

This case is repeated below

),
},
// {
// Name: "Set postUpgrade.labelNamespace.image.pullSecrets with labelNamespace set to true",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

// Name: "Set postUpgrade.labelNamespace.extraNamespaces",

// TemplateOptions: chart.NewTemplateOptions(DefaultReleaseName, DefaultNamespace).
// Set(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these cases supposed to be covered?

@eliyamlevy
Copy link
Contributor

I have pointed out some issues with the PR, aside from the build failing. Please address the issues in ALL locations in the PR not just where I have commented. There is multiple instances of commented code left in, a repeated test case, and potentially not 100% test coverage.

@doflamingo721
Copy link
Contributor Author

The hull testing coverage 100% coverage for the gatekeeper chart is not possible at the moment because of multiple issues in the upstream and rancher's gatekeeper template. Some of the issues are already logged (rancher/rancher#41688 and rancher/rancher#41689) but similar indentation and tolerations from values.yaml not getting applied to the templates issues still persist in other templates as well. Some fixes are already raised in the upstream chart for this (open-policy-agent/gatekeeper#2817 and open-policy-agent/gatekeeper#2862) but these fixes would be released with the new gatekeeper chart. As of now the chart coverage is at 97 percent.
I'd suggest merging the PR at 97 percent coverage once it is approved and with coverage disabled so that it won't affect the ci builds.
@eliyamlevy @gunamata What do you think should be the right approach?

@nicholasSUSE
Copy link
Collaborator

Hello @doflamingo721 and @eliyamlevy
Is this Pull Request still relevant?
Can I close it?

@nicholasSUSE
Copy link
Collaborator

Unresponsive, closing.

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.

4 participants