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

new init prompt function and helper functions #5141

Merged

Conversation

MarlonGamez
Copy link
Contributor

@MarlonGamez MarlonGamez commented Dec 11, 2020

Related: #4915

Description
This PR adds the functions that will be used to implement the transparent init feature.
The main is the ConfirmInitOptions() prompt function.

There are also some new util functions defined in pkg/skaffold/initializer/util

Follow-up Work
Use these functions to implement transparent init

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #5141 (bec4398) into master (35e1bf9) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5141      +/-   ##
==========================================
+ Coverage   72.14%   72.20%   +0.05%     
==========================================
  Files         381      382       +1     
  Lines       13565    13589      +24     
==========================================
+ Hits         9787     9812      +25     
+ Misses       3061     3060       -1     
  Partials      717      717              
Impacted Files Coverage Δ
pkg/skaffold/initializer/prompt/prompt.go 27.45% <100.00%> (+27.45%) ⬆️
pkg/skaffold/initializer/util/util.go 100.00% <100.00%> (ø)
pkg/skaffold/deploy/helm/deploy.go 71.77% <0.00%> (+0.27%) ⬆️
pkg/skaffold/runner/runcontext/context.go 84.61% <0.00%> (+1.28%) ⬆️

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 35e1bf9...bec4398. Read the comment docs.

@MarlonGamez MarlonGamez marked this pull request as ready for review December 11, 2020 22:10
@MarlonGamez MarlonGamez requested a review from a team as a code owner December 11, 2020 22:10
}

// ListDeployers returns a list of deployer names being used in the given deploy config.
func ListDeployers(deploy *latest.DeployConfig) []string {
Copy link
Contributor

@IsaacPD IsaacPD Dec 12, 2020

Choose a reason for hiding this comment

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

Looks good just thought this function might be useful since I used it for the same use case so I think it could help here too by using it as the underlying function. It just uses the yaml that deploy *latest.DeployConfig unmarshals to, to get the list of deployers.

meter.Deployers = yamltags.GetYamlTags(config.Deploy.DeployType)

// GetYamlTags returns the tags of the non-nested fields of the given non-nil value
// If value interface{} is
// latest.DeployType{HelmDeploy: &HelmDeploy{...}, KustomizeDeploy: &KustomizeDeploy{...}}
// then this parses that interface as it's raw yaml:
// kubectl:
// manifests:
// - k8s/*.yaml
// kustomize:
// paths:
// - k8s/
// and returns ["kubectl", "kustomize"]
// empty structs (e.g. latest.DeployType{}) when parsed look like "{}"" and so this function returns []
func GetYamlTags(value interface{}) []string {

Copy link
Contributor

@IsaacPD IsaacPD Dec 12, 2020

Choose a reason for hiding this comment

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

Also now realizing there's a tab right before kubectl in the comment that shouldn't be there 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for sharing this, I think your function is probably what I should use here because it should be fine in the case of a new deployer being added to skaffold

@MarlonGamez MarlonGamez merged commit 9422d28 into GoogleContainerTools:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants