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

[v2] feat: Add kubectl renderer #7118

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Feb 15, 2022

In PR, we are adding a kubectl renderer so user's just relying on kubectl or helm don't have to depend on kpt.

The kubectl renderer is invoked when

  1. there is no validator is defined
  2. no kpt config defined and
  3. No transformer defined.
if config.Validate == nil && config.Transform == nil && config.Kpt == nil {
		return kubectl.New(config, workingDir, hydrationDir, labels)
	}

Description of changes

  1. Move skaffold/render/renderer/renderer.go → pkg/skaffold/render/renderer/kpt/kpt.go and rename renderer.SkaffoldRenderer to renderer.kpt.Kpt
  2. Move skaffold/render/renderer/renderer_test.go → pkg/skaffold/render/renderer/kpt/kpt_test.go
  3. Add renderer.kubectl.Kubectl in pkg/skaffold/render/renderer/kubectl/kubectl.go which uses the already implemented generated.Generator to read k8s manifests, add labels to these manifests and write them to a hydration dir
  4. Create a render util GenerateHydratedManifests which calls generated.Generator.Generate to read k8s manifests, add labels to these manifests and write them to a hydration dir. This method is used in both Kpt and Kubectl renderer.

Things to do next

@tejal29 tejal29 requested a review from a team as a code owner February 15, 2022 23:18
@tejal29 tejal29 requested review from MarlonGamez and removed request for a team February 15, 2022 23:18
@tejal29 tejal29 changed the title feat: Add kubectl renderer [v2] feat: Add kubectl renderer Feb 15, 2022
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v2-v1.36.0@c35b5d1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             v2-v1.36.0    #7118   +/-   ##
=============================================
  Coverage              ?   67.61%           
=============================================
  Files                 ?      557           
  Lines                 ?    26552           
  Branches              ?        0           
=============================================
  Hits                  ?    17954           
  Misses                ?     7310           
  Partials              ?     1288           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c35b5d1...95de416. Read the comment docs.

if err := manifest.Write(manifests.String(), dryConfigPath, out); err != nil {
return err
}
endTrace()
Copy link
Member Author

Choose a reason for hiding this comment

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

copied over to GenerateHydratedManifests in renderer/util package

@tejal29 tejal29 force-pushed the add_kubectl_renderer branch 2 times, most recently from cd198c6 to aa09298 Compare February 16, 2022 20:53
@MarlonGamez
Copy link
Contributor

Hey @tejal29 , can we rebase this on v2-1.36.0?

@tejal29 tejal29 changed the base branch from v2-v1.35.2 to v2-v1.36.0 February 17, 2022 19:00
@tejal29
Copy link
Member Author

tejal29 commented Feb 17, 2022

Hey @tejal29 , can we rebase this on v2-1.36.0?

done!

@MarlonGamez
Copy link
Contributor

@tejal29 looks like there are some linter errors but besides that I think this is good !

@tejal29
Copy link
Member Author

tejal29 commented Feb 22, 2022

@tejal29 looks like there are some linter errors but besides that I think this is good !

done!

@tejal29 tejal29 merged commit 00c4877 into GoogleContainerTools:v2-v1.36.0 Feb 23, 2022
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.

2 participants