search

rancher / charts-build-scripts

Apache License 2.0
9 stars 32 forks source link

make validate should do deep checks for modified tgz files #56

Closed aiyengar2 closed 2 years ago

aiyengar2 commented 3 years ago

In https://github.com/rancher/charts-build-scripts/blob/deb66c8dd574ddd1f3b58271fec97b7b141588d0/pkg/validate/validate.go#L24-L28, we do a simple check to see if Git is clean on comparing generated assets.

However, if the only change that we see is a modified tgz file and the charts contained within those tgz files are the same (e.g. only timestamp differences), we should not fail make validate.

This is the root cause of why we saw a failure on make validate in https://github.com/rancher/charts/runs/3204473035?check_suite_focus=true, where release-v2.5 contained an asset that was generated from dev-v2.5.9 but it was compared against the asset in dev-v2.5. The contents in dev-v2.5.9's version of the chart and dev-v2.5's version of the chart were identical, but validate resulted in a failure since we did not do a deeper check of equality for the tgzs.

cbron commented 3 years ago

Wouldn't this issue be fixed by pulling down the asset from release-v2.5 into dev-v2.5 ?

aiyengar2 commented 3 years ago

Wouldn't this issue be fixed by pulling down the asset from release-v2.5 into dev-v2.5 ?

Yes it would. So if we don't fix this, this would have to be a manual step in every OOB release.

But ideally deep checks is a better / more safe approach since we're also verifying the contents of the TGZ files, which we do not do today.

cbron commented 3 years ago

Wouldn't you want to keep those tgz in line though ? So they have the same hash's and all. I'd rather move in direction where we ensure those branches are in sync. Like the idea of wiping out dev-v2.x on every merge and regenerating with release-v2.6 as base.

aiyengar2 commented 3 years ago

Yes, I think that type of process change works too. But I think the repercussions of that are:

  1. All existing PRs might get invalidated until they rebase on the regenerated feature branch
  2. It puts more burden on the release captain to execute this change after doing the release

But all in all, I think what you are suggesting is a better route to take atm, as long as we are okay with the above two caveats.

cbron commented 3 years ago

Ok, we'll need to go that route for 2.6.