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 yaml tag filepath that converts marked fields to absolute paths. #5194

Closed

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Dec 28, 2020

Fixes: #5204

This PR introduces a new yaml tag filepath for config.go which converts the marked field to an absolute path.

type Artifact struct {
	// Workspace is the directory containing the artifact's sources.
	// Defaults to `.`.
	Workspace string `yaml:"context,omitempty" yamltags:"filepath"`
        ...
}

type KubectlDeploy struct {
	// Manifests lists the Kubernetes yaml or json manifests.
	// Defaults to `["k8s/*.yaml"]`.
	Manifests []string `yaml:"manifests,omitempty" yamltags:"filepath"`
        ...
}

This is useful since the current code assumes all relative paths in the skaffold config are defined from the os.Getwd() directory. With #5160 we'll open up support for multiple skaffold configs. Doing an upfront file path massaging avoids having to maintain the info for base paths for each config individually which can be error prone.

It also fixes this assumption that the current working directory isn't the skaffold config location but the skaffold executable dir.
(comment on context.go)

// TODO(dgageot): this should be the folder containing skaffold.yaml. Should also be moved elsewhere.
cwd, err := os.Getwd()

Followup: After this is merged, we can remove WorkingDir from runcontext and many filepath.Joins strewn throughout the codebase become redundant.

@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #5194 (a1f6868) into master (35214eb) will decrease coverage by 0.06%.
The diff coverage is 65.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5194      +/-   ##
==========================================
- Coverage   71.82%   71.75%   -0.07%     
==========================================
  Files         387      388       +1     
  Lines       13928    14008      +80     
==========================================
+ Hits        10004    10052      +48     
- Misses       3190     3211      +21     
- Partials      734      745      +11     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 58.82% <ø> (ø)
cmd/skaffold/app/cmd/runner.go 62.50% <20.00%> (-4.17%) ⬇️
pkg/skaffold/util/config.go 65.38% <55.55%> (-12.40%) ⬇️
pkg/skaffold/yamltags/paths.go 66.66% <66.66%> (ø)
pkg/skaffold/schema/versions.go 79.66% <100.00%> (-2.49%) ⬇️
pkg/skaffold/util/util.go 83.19% <100.00%> (-0.15%) ⬇️
pkg/skaffold/yamltags/tags.go 89.69% <100.00%> (+0.21%) ⬆️

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 35214eb...a1f6868. Read the comment docs.

@gsquared94
Copy link
Contributor Author

something's wrong with the k3d integration tests. All other PRs are failing as well.

@@ -37,6 +37,8 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/validation"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/update"
skutil "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we usually call this pkgutil i think

switch parentStruct.Kind() {
case reflect.Struct:
t := parentStruct.Type()
var errs []error
Copy link
Contributor

Choose a reason for hiding this comment

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

did my yearly reading on handling multiple errors in go, and i realized something i hadn't before: errors.Wrap is actually meant to handle this. https://golang.org/doc/go1.13#error_wrapping

the idea is that for each successive error you encounter, you wrap it in the previous error: errors.Wrap(prevErr, newErr).

then later when you get that fully wrapped error, you continuously call errors.Unwrap until you get nil - do that in a while loop and you can unwind the errors, like popping them off a stack.

anyway, multiple error handling in go is stupid any way you slice it so i'm not gonna say you should do this in this PR, but just wanted to share my revelation :)

@gsquared94
Copy link
Contributor Author

closing in favor of #5205. We retain the current behavior of CWD being the base dir.

@gsquared94 gsquared94 closed this Jan 6, 2021
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.

Relative paths in skaffold.yaml are interpreted relative to cwd instead of the file directory
2 participants