-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: kustomize render should support components #9636
fix: kustomize render should support components #9636
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:
This pull request, authored by kallangerard, addresses issue #9635 and is related to #9239. The goal is to enhance Skaffold's kustomize render
command to correctly support Kustomize components.
Intent: The primary intent is to fix a bug where skaffold render
failed when encountering Kustomize components within the kustomization.yaml
file. Previously, the command would fail to process these components, resulting in an error. This PR ensures that components are correctly included in the rendering process.
Changes:
The changes involve modifications to the skaffold render
command's handling of Kustomize files. Specifically:
integration/render_test.go
: Several test cases were updated to verify the correct handling of Kustomize components. These changes include additions to existing test cases (lines 1438-1468, 1460-1478, 2064-2083, 2332-2351, 2342-2361) and address whitespace issues (lines 2064-2083, 2332-2351, 2342-2361).pkg/skaffold/render/renderer/kustomize/kustomize.go
: New functionality was added (lines 211-216, 405-430) to handle the mirroring of Kustomize components during the rendering process. This involved creating a new functionmirrorComponents
to recursively process component directories and files.
Summary Table:
File | Change Summary |
---|---|
integration/render_test.go |
Updated test cases to include Kustomize components and fix whitespace issues. |
pkg/skaffold/render/renderer/kustomize/kustomize.go |
Added mirrorComponents function to handle mirroring of Kustomize components, fixing the issue where components were not processed correctly. |
This should provide a good overview of the changes. I'll post a more detailed review shortly.
And now, a little haiku to lighten the mood:
Components now seen,
Skaffold renders, clear and bright,
Bugs flee in the night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses the issue of kustomize components not being included in the skaffold render
process. The approach of introducing a mirrorComponents
function is well-structured and effectively handles both file and directory components. The included tests appropriately demonstrate the fix and ensure the correct rendering of components. Overall, the changes are well-implemented and improve the functionality of skaffold render
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request, submitted by @kallangerard, aims to fix an issue where skaffold render
didn't properly support Kustomize components. The core problem was that the skaffold render
command failed to correctly process Kustomize components when rendering manifests.
Intent:
The goal is to enable skaffold render
to correctly handle Kustomize components, allowing users to utilize them in their manifests without encountering errors.
Changes:
The changes involve modifications to the skaffold render
command's handling of Kustomize components. Specifically:
integration/render_test.go
: Several test cases were updated to verify the correct functioning of Kustomize components within the rendering process. These changes include additions and modifications to test data and expected outputs, ensuring comprehensive test coverage of the fix. Lines 1438-1468, 1460-1478, 2064-2083, 2332-2351, 2342-2361 were modified.pkg/skaffold/render/renderer/kustomize/kustomize.go
: The core logic for handling Kustomize components was added. A new functionmirrorComponents
was introduced (lines 411-430) to recursively process components, mirroring them into a temporary file system. This ensures that components are correctly included in the rendering process. Existing functions were also modified to integrate with this new component handling logic. Lines 211-216 and 405-409 were also modified.
Activity:
The pull request addresses issue #9635 and is related to issue #9239. The description includes before-and-after examples demonstrating the resolution of the rendering error.
I'll post a more detailed review shortly.
And now, a little haiku to lighten the mood:
Code flows like a stream,
Components merge, a clear fix,
Skaffold builds with glee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses the issue of kustomize components not being included in the skaffold render
mirroring process. The approach of mirroring components, similar to how configMapGenerators are handled, is sound. The changes are well-implemented and the included test cases are helpful. A minor suggestion would be to add a test case that specifically uses a component that is a file rather than a directory, just to ensure complete coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:
This pull request, authored by kallangerard, addresses issue #9635 and is related to #9239. The goal is to enable skaffold render
to correctly handle Kustomize components.
Previously, skaffold render
failed when encountering Kustomize components within a kustomization.yaml
file. This PR fixes that issue.
Here's a breakdown of the changes:
integration/render_test.go
: Several test cases were updated to verify the correct handling of Kustomize components. Specific changes include additions to test cases around lines 1438, 1460, 2064, 2332, 2342, ensuring that components are properly rendered and included in the output. Whitespace changes were also made around lines 2064, 2332, 2342, and 2355.pkg/skaffold/render/renderer/kustomize/kustomize.go
: The core logic was modified to recursively process Kustomize components. A new functionmirrorComponents
was added around line 405 to handle the mirroring of these components. This function iterates through the components, checks if they are directories or files, and mirrors them accordingly.
The changes ensure that Kustomize components are correctly included in the rendering process, resolving the original error.
I'll provide a more detailed review shortly.
And now, a little haiku to lighten the mood:
Components now flow,
Skaffold renders, clear and bright,
No more error woes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request addresses the issue of kustomize components not being included in the skaffold render
process. The approach of mirroring the components directory is a good solution. The changes are well-implemented and the tests effectively demonstrate the fix. Adding a test case for a component that is a single file would further enhance test coverage.
a042d16
to
841ab37
Compare
…ng a kustomization file
Fixes: #9635
Related: #9239
Description
Include Kustomize Components in mirroring for
skaffold render
.Before
After
/Users/kallangerard/projects/public/skaffold/out/skaffold render -p with-components --set="foo=bar" apiVersion: apps/v1 kind: Deployment metadata: labels: app: hello service: helloworld name: helloweb spec: selector: matchLabels: app: hello tier: web template: metadata: labels: app: hello service: helloworld tier: web spec: containers: - image: helloworld name: hello-app