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 regression in changed chart detection #85

Merged
merged 4 commits into from
Jan 20, 2019

Conversation

unguiculus
Copy link
Member

Fixes #84

 Please enter the commit message for your changes. Lines starting

Signed-off-by: Reinhard Nägele <[email protected]>
@unguiculus unguiculus force-pushed the fix-change-detection branch from 0392068 to d4e63ae Compare January 17, 2019 18:03
@helm-bot helm-bot added size/XS and removed size/XS labels Jan 17, 2019
@Flydiverny
Copy link
Contributor

Seems good to me 👍

Tested:

  • changing something in templates/service.yaml -> lint detects and asks for version bump
  • changing maintainer in Chart.yaml -> lint detects and asks for version bump
  • changing templates/service.yaml & version in Chart.yaml -> lint detects and passes

@Flydiverny
Copy link
Contributor

Also tested setting chart-dirs as . (root), here it doesn't seem to detect changes with this revert. Does detect with 2.0.1 tho.

------------------------------------------------------------------------------------------------------------------------
 Configuration
------------------------------------------------------------------------------------------------------------------------
Remote: origin
TargetBranch: master
BuildId:
LintConf: lintconf.yaml
ChartYamlSchema: chart_schema.yaml
ValidateMaintainers: true
ValidateChartSchema: true
ValidateYaml: true
CheckVersionIncrement: true
ProcessAllCharts: false
Charts: []
ChartRepos: []
ChartDirs: [.]
ExcludedCharts: []
HelmExtraArgs: --timeout 800
HelmRepoExtraArgs: []
Debug: true
Namespace:
ReleaseLabel:
------------------------------------------------------------------------------------------------------------------------
>>> git merge-base origin/master HEAD
>>> git diff --find-renames --name-only b00fdb7fcc0dbd813517b6523cdecb0a6c01d68b -- .
All charts linted successfully
------------------------------------------------------------------------------------------------------------------------
No chart changes detected.
------------------------------------------------------------------------------------------------------------------------
λ git diff --find-renames --name-only b00fdb7fcc0dbd813517b6523cdecb0a6c01d68b -- .
ct.yaml
notifications/.helmignore
notifications/Chart.yaml
notifications/templates/NOTES.txt
notifications/templates/_helpers.tpl
notifications/templates/deployment.yaml
notifications/templates/job.yaml
notifications/templates/pdb.yaml
notifications/templates/service.yaml
notifications/values.yaml

@Flydiverny
Copy link
Contributor

Flydiverny commented Jan 17, 2019

		dir := path.Join(pathElements[0], pathElements[1])
		rootDir := filepath.Dir(file)
		// Only add if not already in list and double-check if it is a chart directory
		if !util.StringSliceContains(changedChartDirs, dir) && t.chartUtils.IsChartDir(dir) {
			changedChartDirs = append(changedChartDirs, dir)
		} else if !util.StringSliceContains(changedChartDirs, rootDir) && t.chartUtils.IsChartDir(rootDir) {
			changedChartDirs = append(changedChartDirs, rootDir)
		}

Works for both cases, even tho its not so pretty 😄

Signed-off-by: Reinhard Nägele <[email protected]>
@helm-bot helm-bot added size/M and removed size/XS labels Jan 18, 2019
@unguiculus
Copy link
Member Author

I refactored it a little bit. Could you test this again? I just quickly made the tests pass but would like to improve those as well.

Copy link
Contributor

@Flydiverny Flydiverny left a comment

Choose a reason for hiding this comment

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

Tested a bit again:

λ ~/Code/Go/bin/app lint --debug
Linting charts...
Using config file:  /Users/flydiverny/Code/gleerups/chart-repository/ct.yaml
------------------------------------------------------------------------------------------------------------------------
 Configuration
------------------------------------------------------------------------------------------------------------------------
Remote: origin
TargetBranch: master
BuildId:
LintConf: lintconf.yaml
ChartYamlSchema: chart_schema.yaml
ValidateMaintainers: true
ValidateChartSchema: true
ValidateYaml: true
CheckVersionIncrement: true
ProcessAllCharts: false
Charts: []
ChartRepos: []
ChartDirs: [.]
ExcludedCharts: []
HelmExtraArgs: --timeout 800
HelmRepoExtraArgs: []
Debug: true
Namespace:
ReleaseLabel:
------------------------------------------------------------------------------------------------------------------------
>>> git merge-base origin/master HEAD
>>> git diff --find-renames --name-only b00fdb7fcc0dbd813517b6523cdecb0a6c01d68b -- .

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 notifications
 notifications
 notifications
 notifications
 notifications
 notifications
 notifications
------------------------------------------------------------------------------------------------------------------------

Detects changes for both charts in root dir and a "normal" setup with eg stable folder.
However if multiple files are changed for the same chart it will lint it multipletimes.

@@ -424,8 +424,13 @@ func (t *Testing) ComputeChangedChartDirectories() ([]string, error) {
}
dir := filepath.Dir(file)
// Only add if not already in list and double-check if it is a chart directory
if !util.StringSliceContains(changedChartDirs, dir) && t.chartUtils.IsChartDir(dir) {
changedChartDirs = append(changedChartDirs, dir)
if !util.StringSliceContains(changedChartDirs, dir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change order here and check if chartDir is added already. Otherwise we will get multiple entries if there are multiple files changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work:

		chartDir, err := t.chartUtils.LookupChartDir(cfg.ChartDirs, dir)
		if err == nil {
			if !util.StringSliceContains(changedChartDirs, chartDir) {
				changedChartDirs = append(changedChartDirs, chartDir)
			}
		} else {
			fmt.Printf("Directory '%s' is not chart directory. Skipping...", chartDir)
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed as suggested. I figured more elaborate test would no be easy. I'd leave them as is for now.

@helm-bot helm-bot added size/M and removed size/M labels Jan 18, 2019
Signed-off-by: Reinhard Nägele <[email protected]>
@helm-bot helm-bot added size/M and removed size/M labels Jan 18, 2019
Copy link
Contributor

@Flydiverny Flydiverny left a comment

Choose a reason for hiding this comment

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

tested last update works as far as I can tell :)

@scottrigby
Copy link
Member

I also tested the same cases in #85 (comment) and it works 👍

@scottrigby scottrigby merged commit b679b85 into helm:master Jan 20, 2019
@unguiculus unguiculus deleted the fix-change-detection branch January 21, 2019 13:19
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