Skip to content

feat: add checking in remote repo if manifest is a chart #3811

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

a1994sc
Copy link
Contributor

@a1994sc a1994sc commented May 15, 2025

Description

Makes call to zarf registry when trying to mutate ocirepo resource, and checks if the manifest is a helm chart

Related Issue

Fixes #3435

Checklist before merging

Copy link

netlify bot commented May 15, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 1a89c64
🔍 Latest deploy log https://app.netlify.com/projects/zarf-docs/deploys/68434f2576f1150008f039a8

@a1994sc a1994sc marked this pull request as ready for review May 19, 2025 18:55
@a1994sc a1994sc requested review from a team as code owners May 19, 2025 18:55
@a1994sc
Copy link
Contributor Author

a1994sc commented May 19, 2025

Hey team, I believe that this PR is in a better state to be reviewed. I have fixed the unit tests to maintained backwards-compatibility with the current logic, e.i. if A) it can not reach the zarf registry, or B) if the media type is not of type helm. I also included a new unit test for checking with an in-memory registry, shoot-out to @brandtkeller for the awesome help both here in the PR and in DM's .

Please let me know if there are any changes you would like to make

Edit:

I will be also updating the docs and what not, so well the code stuff is ready for review the UX/docs needs to be updated

@a1994sc a1994sc requested a review from brandtkeller May 22, 2025 20:33
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

This is an excellent start, thank you for doing research and driving this forward!

Copy link

codecov bot commented May 27, 2025

Codecov Report

Attention: Patch coverage is 81.48148% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/agent/hooks/common.go 80.55% 5 Missing and 2 partials ⚠️
src/internal/agent/hooks/flux-ocirepo.go 83.33% 2 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
src/internal/agent/hooks/flux-ocirepo.go 81.73% <83.33%> (+0.29%) ⬆️
src/internal/agent/hooks/common.go 83.33% <80.55%> (-16.67%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@a1994sc
Copy link
Contributor Author

a1994sc commented May 27, 2025

This is an excellent start, thank you for doing research and driving this forward!

No problem at all!

@a1994sc a1994sc requested a review from AustinAbro321 May 28, 2025 19:54
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

A few nits, and some test cleanup to be done. Overall changes are looking good.

@@ -35,6 +35,111 @@ func createFluxOCIRepoAdmissionRequest(t *testing.T, op v1.Operation, fluxOCIRep
}
}

func TestFluxOCIHelmMutationWebhook(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a new function here we should move these into the existing function. We actually want the registry to be setup for the other test as currently the test takes an extra 10 seconds so it can timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh, the separate function was mostly so that I can focus my tests on the function I was creating, so I can merge that back into one function without too much issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I update the tests into one, had to change some of the existing tests to match the new url.... but I'm having issues with the test that use a custom svc such as here:

svc: &corev1.Service{
TypeMeta: metav1.TypeMeta{
APIVersion: corev1.SchemeGroupVersion.String(),
Kind: "Service",
},
ObjectMeta: metav1.ObjectMeta{
Name: "zarf-docker-registry",
Namespace: "zarf",
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeNodePort,
Ports: []corev1.ServicePort{
{
NodePort: int32(31999),
Port: 5000,
},
},
ClusterIP: "10.11.12.13",
},
},

Any input on how to update the tests would be great!

@a1994sc a1994sc requested a review from AustinAbro321 June 2, 2025 20:30
@a1994sc a1994sc force-pushed the feat/oci-repo branch 3 times, most recently from 9bac17c to db059bf Compare June 6, 2025 19:55
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.

FluxCD helm release fails when using ocirepo resource as chart reference
3 participants