clean chart cache dir on error processing

[prydonius]

No description provided.

[codecov-io]

Codecov Report

Merging #281 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #281      +/-   ##
=========================================
+ Coverage   90.35%   90.4%   +0.04%     
=========================================
  Files          18      18              
  Lines         964     969       +5     
=========================================
+ Hits          871     876       +5     
  Misses         62      62              
  Partials       31      31

Impacted Files	Coverage Δ
...api/data/cache/charthelper/chart_package_helper.go	79.28% <100%> (+0.76%)	⬆️

---

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 3f5bf20...61e891a. Read the comment docs.

[jbianquetti-nami]

LGTM

[mkmik]

why is this a variable? do you intend to replace it?

or do you do that for every function just in case?

[prydonius]

This is something that we're doing for functions that need to be overridden in tests, although I'm not actually overriding it in this case. I'm not sure if it makes sense to only use a variable where we intend to test it that way. What do you think?

[mkmik]

why do you want to return an error if the dir doesn't exist?

i.e. I wonder why cannot this just be:

var cleanChartDataDir = func(chart *models.ChartPackage) error {
  return os.RemoveAll(chartDataDir(chart))
}

[prydonius]

Fair point, I was copying the same structure for ensureDataDir but I think you're right in that this could be much simpler.

[mkmik]

Just a FYI:

var DownloadAndExtractChartTarball = func(chart *models.ChartPackage) (err error) {
 	if err := ensureChartDataDir(chart); err != nil {
 		return err
 	}

 	defer func() {
 	 	if err != nil {
 	 	 	cleanChartDataDir(chart)
 	 	}
 	}()

 	if !tarballExists(chart) {
  		if err := downloadTarball(chart); err != nil {
			return err
  		}
  	}

 	if err := extractFilesFromTarball(chart); err != nil {
  		return err
  	}
  
 	return nil
}

this ensures that if you add another place where you return an error you cannot forget to delete the char data dir when the error happens.

[prydonius]

Oh nice, I was trying to figure out how I could do this, will change it to this - thanks!