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

Add Initializer Interface to skaffold to support other deployers in skaffold init #1756

Merged
merged 5 commits into from
Mar 12, 2019

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Mar 7, 2019

This PR is implementing the Proposal mentioned in #1726

In this PR,

  1. Added a new Interface
type Initializer interface {
	// GenerateDeployConfig generates Deploy Config for skaffold configuration.
	GenerateDeployConfig() latest.DeployConfig
	// GetImages fetches all the images defined in the manifest files.
	GetImages() []string
}

  1. Moved all the Kubectl deployer specific code to "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/kubectl" module and added tests.

  2. Moved all skaffold init processing from cmd/skaffold/app/cmd/init.go to pkg/skaffold/initializer/init.go and corresponding tests.
    In order to keep the diff minimum, i have not refactored a lot of code in to modules. I will refactor funcs like "resolveDockerfileImages" or "processCLIArtifacts" as appropriate when i add other intializers.

  3. Added tests for some utility functions.

@tejal29 tejal29 removed the request for review from r2d4 March 7, 2019 23:09
@tejal29 tejal29 force-pushed the add_initializer_module branch 2 times, most recently from 3da9c35 to d3a71d8 Compare March 8, 2019 00:25
@balopat balopat self-assigned this Mar 8, 2019
@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #1756 into master will increase coverage by 0.19%.
The diff coverage is 28.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1756      +/-   ##
==========================================
+ Coverage   46.27%   46.46%   +0.19%     
==========================================
  Files         137      140       +3     
  Lines        6492     6523      +31     
==========================================
+ Hits         3004     3031      +27     
- Misses       3188     3189       +1     
- Partials      300      303       +3
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/init.go 0% <0%> (-18.81%) ⬇️
pkg/skaffold/initializer/util.go 100% <100%> (ø)
pkg/skaffold/initializer/init.go 6.45% <6.45%> (ø)
pkg/skaffold/initializer/kubectl/kubectl.go 77.77% <77.77%> (ø)

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 b0f4b03...7aff98d. Read the comment docs.

@balopat balopat changed the title Add Initializer Interface to skaffold to support other deply tools in skaffold init flow. Add Initializer Interface to skaffold to support other deployers in skaffold init flow Mar 8, 2019
@balopat balopat changed the title Add Initializer Interface to skaffold to support other deployers in skaffold init flow Add Initializer Interface to skaffold to support other deployers in skaffold init Mar 8, 2019
// IsSupportedKubernetesFormat is for determining if a file under a glob pattern
// is deployable file format. It makes no attempt to check whether or not the file
// is actually deployable or has the correct contents.
func IsSupportedKubernetesFormat(n string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func IsSupportedKubernetesFormat(n string) bool {
func IsSupportedKubernetesFileExtension(n string) bool {

?

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@tejal29 tejal29 merged commit cd2bd4a into GoogleContainerTools:master Mar 12, 2019
@tejal29 tejal29 deleted the add_initializer_module branch April 15, 2021 07:33
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.

4 participants