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

fix logic that set absolute paths in the parsed configuration #5452

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Feb 25, 2021

Fixes: #5396

Test

Setup the files

├── frontend
│   └── web-app
│       └── skaffold.yaml
└── skaffold.yaml

where:
/skaffold.yaml:

apiVersion: skaffold/v2beta12
kind: Config
metadata:
  name: hikingo

requires:
- path: ./frontend/web-app

frontend/web-app/skaffold.yaml:

apiVersion: skaffold/v2beta12
kind: Config
metadata:
  name: web-app
build:
  artifacts:
  - image: xyz
deploy:
  helm:
    releases:
    - name: web-app
      chartPath: ../../infra/charts/microservice
      artifactOverrides:
        image: xyz
      valuesFiles:
      - ./helm-values.yaml

From root directory run:

skaffold diagnose --yaml-only

Before fix Output

...
deploy:
  helm:
    releases:
    - name: web-app
      chartPath: /Users/gaghosh/Code/Hack/repro-5396/infra/charts/microservice
      valuesFiles:
      - ./helm-values.yaml      // <==== failed to set absolute path here
...

After fix Output

...
deploy:
  helm:
    releases:
    - name: web-app
      chartPath: /Users/gaghosh/Code/Hack/repro-5396/infra/charts/microservice
      valuesFiles:
      - /Users/gaghosh/Code/Hack/repro-5396/frontend/web-app/helm-values.yaml
...

@gsquared94 gsquared94 requested a review from a team as a code owner February 25, 2021 05:18
@google-cla google-cla bot added the cla: yes label Feb 25, 2021
@gsquared94 gsquared94 requested a review from tejal29 February 25, 2021 05:18
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #5452 (e7405a4) into master (7f2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5452   +/-   ##
=======================================
  Coverage   71.41%   71.42%           
=======================================
  Files         397      397           
  Lines       14551    14547    -4     
=======================================
- Hits        10392    10390    -2     
+ Misses       3388     3387    -1     
+ Partials      771      770    -1     
Impacted Files Coverage Δ
pkg/skaffold/tags/paths.go 73.21% <100.00%> (+1.54%) ⬆️

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 7f2eeed...e7405a4. Read the comment docs.

if filepath.IsAbs(path) {
return errs
if path == "" || filepath.IsAbs(path) {
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was erroneously returning here. That meant if two successive fields in a struct have the filepath tag (like HelmRelease) and the first is set to an absolute value already, then the second field did not get processed.

Copy link
Contributor

@MarlonGamez MarlonGamez left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@MarlonGamez MarlonGamez merged commit 27d1de2 into GoogleContainerTools:master Feb 25, 2021
@gsquared94 gsquared94 added this to the v1.21.0 milestone Feb 28, 2021
@ReneWerner87
Copy link

ReneWerner87 commented Mar 11, 2021

@gsquared94 what about the other helm configuration options ? like the setFiles part?
https://skaffold.dev/docs/references/yaml/#deploy-helm-releases-setFiles
the problem still exists there

By the way, the feature with the multi repos is mega cool(thx for the effort), wanted to use it, but this issue and the part with the setFiles are blocking me.

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.

Unexpected base-path for required skaffold's helm.setValues
3 participants