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: Skaffold post renderer #9203

Merged

Conversation

ericzzzzzzz
Copy link
Contributor

@ericzzzzzzz ericzzzzzzz commented Dec 4, 2023

Fixes: #9038

Description

  • enable users to use skaffold pipeline post-render host hooks as helm post-render, so they can manipulate skaffold rendered manifests with any renderers
  • To do this, we need to introduce a flag to let users explicitly indicate that they want to leverage manifests from skaffold render phase for further processing. This may not be obvious for render command, but for dev is necessary.
  • We also need to disable stdin logging on post render hook, this is necessary regardless we introduce this feature or now, as enable logging there could lead to unexpected output to end rendered result when using skaffold render > somefile.txt
  • refactor share hooks run method to return skip error, if command doesn't match host machine, we should distinguish a command is successfully finished or just skipped.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 295 lines in your changes are missing coverage. Please review.

Comparison is base (290280e) 70.48% compared to head (2ee41d4) 63.63%.
Report is 1083 commits behind head on main.

Files Patch % Lines
cmd/skaffold/app/cmd/exec.go 16.32% 40 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/filter.go 47.27% 22 Missing and 7 partials ⚠️
cmd/skaffold/app/cmd/lsp.go 28.12% 23 Missing ⚠️
cmd/skaffold/app/cmd/verify.go 23.33% 23 Missing ⚠️
cmd/skaffold/app/cmd/fix.go 51.16% 17 Missing and 4 partials ⚠️
cmd/skaffold/app/cmd/inspect_job_manifest_paths.go 60.00% 15 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/inspect_namespaces.go 50.00% 13 Missing and 1 partial ⚠️
...md/skaffold/app/cmd/inspect_config_dependencies.go 45.83% 12 Missing and 1 partial ⚠️
cmd/skaffold/app/cmd/lint.go 42.85% 12 Missing ⚠️
cmd/skaffold/app/cmd/inspect_build_env.go 60.71% 11 Missing ⚠️
... and 21 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9203      +/-   ##
==========================================
- Coverage   70.48%   63.63%   -6.85%     
==========================================
  Files         515      632     +117     
  Lines       23150    32601    +9451     
==========================================
+ Hits        16317    20747    +4430     
- Misses       5776    10257    +4481     
- Partials     1057     1597     +540     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ericzzzzzzz ericzzzzzzz marked this pull request as ready for review December 14, 2023 16:24
integration/render_test.go Outdated Show resolved Hide resolved
kind: Pod
metadata:
labels:
app1: after-change-1
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, I think this should be app1: before-change-1? 🤔 Looks like the test reports it passed due to something related with the string with multiple yaml documents and the yaml.Unmarshal function (used in CheckDeepEqual); according with the docs yaml.Unmarshal takes only the first document in the string...I think 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh , great catch ! fixed the test and changed the testing helper as well.

integration/examples/lifecycle-hooks/skaffold.yaml Outdated Show resolved Hide resolved
pkg/skaffold/render/renderer/render_mux.go Show resolved Hide resolved
pkg/skaffold/render/renderer/render_mux.go Show resolved Hide resolved
pkg/skaffold/hooks/render.go Outdated Show resolved Hide resolved
pkg/skaffold/hooks/render_test.go Outdated Show resolved Hide resolved
pkg/skaffold/hooks/render_test.go Outdated Show resolved Hide resolved
pkg/skaffold/hooks/render_test.go Show resolved Hide resolved
@ericzzzzzzz ericzzzzzzz merged commit f83583b into GoogleContainerTools:main Jan 2, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make render output available in post-render hook
2 participants