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

Require charts to be in direct subdirectories of configured charts dirs #90

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

unguiculus
Copy link
Member

@unguiculus unguiculus commented Jan 23, 2019

Fixes a regression introduced in #73 so that chart directories were no longer required to be in a direct subdirectory of configured chart directories.

This caused problems in helm/charts#10830.

cc @munnerz

@unguiculus unguiculus force-pushed the charts-in-subdirectory branch from c880944 to 119e0c4 Compare January 23, 2019 13:21
@helm-bot helm-bot added size/M and removed size/M labels Jan 23, 2019
@unguiculus
Copy link
Member Author

cc @Flydiverny @kunickiaj In case you're around, would you mind testing since it's related to #73 and #85?


// check directory has a Chart.yaml and that it is in a
// direct subdirectory of a configured charts directory
if FileExists(chartYaml) && (parent == chartDir) {
Copy link

Choose a reason for hiding this comment

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

Can chartDir ever be a non-relative path and/or is parent absolute or relative?

Could there be situations where chartDir == /my-dir/charts and parent == charts?

Copy link
Member Author

Choose a reason for hiding this comment

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

parent is always relative because git diff --name-only returns relative paths. chartDir could indeed be absolute. I just tested this and no changed directories would be detected in this case. The directory is expected to be relative to the working directory. I guess I should validate this.

@munnerz
Copy link

munnerz commented Jan 23, 2019

Testing this locally worked for me:

$ ct lint --chart-dirs deploy/charts
------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 deploy/charts/cert-manager
------------------------------------------------------------------------------------------------------------------------

(where deploy/charts/cert-manager/webhooks contains a Chart.yaml file).

Without this patch, both the cert-manager and webhook chart are processed.

@munnerz
Copy link

munnerz commented Jan 23, 2019

/lgtm

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

@unguiculus unguiculus merged commit 3742279 into helm:master Jan 23, 2019
@unguiculus unguiculus deleted the charts-in-subdirectory branch January 23, 2019 14:26
@Flydiverny
Copy link
Contributor

Tested a bit as well, work as expected to me:)
even combining root chart dir and nested 😅

remote: origin
target-branch: master
chart-dirs:
  - .
  - infrastructure

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.

6 participants