Deprecate providesGVR flag and update validate script #620

aiyengar2 · 2020-09-09T02:16:19Z

This PR deprecates the provideGVR flag and updates the validate script to unblock CI issues for local charts that use the generateCRDChart flag.

Related to build failures in #594

aiyengar2 · 2020-09-09T02:40:17Z

scripts/validate

+        providesGVR="$(cat ${f}/package.yaml | yq r - providesGVR)"
+		if [[ -n ${providesGVR} && -f ${f}/charts/Chart.yaml ]]; then
+			if [[ "$(yq r ${f}/charts/Chart.yaml 'annotations[catalog.cattle.io/provides-gvr]')" == "${providesGVR}" ]]; then
+				yq d -i ${f}/charts/Chart.yaml "annotations[catalog.cattle.io/provides-gvr]" 


@mrajashree I pulled out your local changes into my fork and tested this out and it seems to be passing validate on your script as expected... with one exception. I had to make a tiny change to your chart in this commit: aiyengar2@e152e52.

The technical reason why this change is required is because yq doesn't support preserving the formatting with in place writing (based on mikefarah/yq#465), so changes like yq d -i or yq w -i sprinkled throughout our codebase essentially reformat your Chart.yaml's formatting to a standard YAML format.

Specifically, this causes an issue in the validate script since it checks to see if your Git is dirty when performing the validate (i.e. you have changes that have not been committed to your patch or directly to your chart). Since you saved your Chart.yaml with a different format than what the output of yq [d/w] -i ${f}/charts/Chart.yaml would have generated, the validate will always fail since it will always assume your git is dirty.

TLDR, if you see an issue with the make validate, just make sure the Chart.yaml's output matches the output provided by yq r ${f}/charts/Chart.yaml, where f=packages/backup-restore-operator or something similar, and run make validate once more.

yup I'd noticed that too, tested my commit with this PR and the ci works fine 👍

@aiyengar2 If yq is going to be changing formatting of files then failing a build this needs to be more obvious, and we need to include formatting as part of the make steps a dev will run locally similar to go-fmt so anyone will be compliant.

Easy solution is just to remove these from package.yaml altogether and just manually put in the patch file. The dual way was too confusing. Since its disconnected from the CRD generation and pushed onto parent it doesn't make sense in package.yaml anymore and will greatly simplify scripts.

@dramich the new last commit on the PR bf9a622 ensures that yq won't break the build. @mrajashree with the last commit, you should no longer need to make the Chart.yaml change to get your chart to pass CI.

I tried adding formatting as part of the make validate instead, but it seems like there are discrepancies between the style yq r gives on the machine that runs our tests vs. the local dev environment (i.e. make validate with some Chart.yamls that I changed to correctly format on my machine failed on the test machine since it expected a slightly different style, such as no spaces on list entries, breaks between big lines, etc.), so unless we somehow standardize the yq style across both environment, a make validate-fmt-type script will be difficult to create. Imo we should avoid making those types of changes in this PR though since this is blocking CIS / backup-restore-operator CI. We can address this when we switch to using Go scripts after 2.5.

re: @cbron's comments, I agree that providesGVR should be removed from the package.yaml to simplify the scripts, but I think we still need to modify the Chart.yaml for auto-install annotations for CRD charts.

We talked about this offline and to simplify things we are just removing all the providesGVR and git update-index stuff. Scripts has to stay small and simple until we rewrite. The downside is that validate might fail for original charts on chart.yaml formatting, we can make a separate make command for that later, but right now its a nice-to-have.

mrajashree

LGTM

cbron

Even if approved don't merge this until after 10am PST.

scripts/clean-crds

cbron · 2020-09-09T15:16:53Z

scripts/validate

+        providesGVR="$(cat ${f}/package.yaml | yq r - providesGVR)"
+		if [[ -n ${providesGVR} && -f ${f}/charts/Chart.yaml ]]; then
+			if [[ "$(yq r ${f}/charts/Chart.yaml 'annotations[catalog.cattle.io/provides-gvr]')" == "${providesGVR}" ]]; then
+				yq d -i ${f}/charts/Chart.yaml "annotations[catalog.cattle.io/provides-gvr]" 


Easy solution is just to remove these from package.yaml altogether and just manually put in the patch file. The dual way was too confusing. Since its disconnected from the CRD generation and pushed onto parent it doesn't make sense in package.yaml anymore and will greatly simplify scripts.

scripts/clean

Before, the logic for reverting a CRD chart was located in `generate-patch` since it was the only script that required these changes. Now that the same logic is required on a `make clean` in order to support reverting just the generateCRDChart changes from local charts, this commit moves that logic out to its own script `clean-crds`. Based on discussion in rancher#607 (comment).

This commit deprecates the providesGVR flag used by charts in favor of charts adding this annotation directly to the patch of their chart (or their Chart.yaml).

By utilizing the `clean-crds` script, both `validate` and `clean` can cleanly deal with issues related to annotations added and files overlaid as part of the CRD chart process.

aiyengar2 · 2020-09-09T19:37:11Z

Chart changes for deprecated providesGVR flag are located in #624

cbron

lgtm, merging this will break the provides->requires dependency but then 624 will fix them again, so we will merge in succession.
The formatting issue will be a known issue with original charts, both current chart owners know and we'll have fix in next edition.

aiyengar2 requested review from mrajashree, cbron and brendarearden September 9, 2020 02:28

aiyengar2 commented Sep 9, 2020

View reviewed changes

aiyengar2 changed the title ~~Update all scripts to properly roll back changes~~ Update several scripts to roll back changes from optional flags Sep 9, 2020

aiyengar2 changed the title ~~Update several scripts to roll back changes from optional flags~~ Update scripts to roll back changes from optional flags Sep 9, 2020

aiyengar2 changed the title ~~Update scripts to roll back changes from optional flags~~ Update scripts to roll back optional flag changes Sep 9, 2020

aiyengar2 force-pushed the fix_local_split_crd_validate branch 2 times, most recently from 4b11820 to 9f0383a Compare September 9, 2020 05:04

aiyengar2 mentioned this pull request Sep 9, 2020

Add generateCRDChart.assumeOwnershipOfCRDs flag #595

Merged

aiyengar2 force-pushed the fix_local_split_crd_validate branch 4 times, most recently from 8e79ff4 to 49eda64 Compare September 9, 2020 06:34

mrajashree approved these changes Sep 9, 2020

View reviewed changes

cbron reviewed Sep 9, 2020

View reviewed changes

aiyengar2 force-pushed the fix_local_split_crd_validate branch 5 times, most recently from 34f65c7 to cb5e5f1 Compare September 9, 2020 16:55

cbron mentioned this pull request Sep 9, 2020

[rancher/charts scripts] RFE: Support CRD charts for Rancher Original charts rancher/rancher#28628

Closed

aiyengar2 force-pushed the fix_local_split_crd_validate branch 4 times, most recently from 94cfbe5 to bf9a622 Compare September 9, 2020 17:29

aiyengar2 requested review from dramich and cbron September 9, 2020 17:48

aiyengar2 force-pushed the fix_local_split_crd_validate branch from bf9a622 to e9d6a4c Compare September 9, 2020 19:05

aiyengar2 force-pushed the fix_local_split_crd_validate branch 3 times, most recently from ecf8e46 to df0cbe7 Compare September 9, 2020 19:24

Arvind Iyengar added 3 commits September 9, 2020 12:25

Deprecate providesGVR flag

a84d714

This commit deprecates the providesGVR flag used by charts in favor of charts adding this annotation directly to the patch of their chart (or their Chart.yaml).

Update validate and clean script to use clean-crds

fb3fe85

By utilizing the `clean-crds` script, both `validate` and `clean` can cleanly deal with issues related to annotations added and files overlaid as part of the CRD chart process.

Fix url check when package.yaml does not exist

b3f18ae

aiyengar2 force-pushed the fix_local_split_crd_validate branch from df0cbe7 to b3f18ae Compare September 9, 2020 19:27

aiyengar2 mentioned this pull request Sep 9, 2020

Add providesGVR to patch file and remove from package.yaml #624

Merged

aiyengar2 changed the title ~~Update scripts to roll back optional flag changes~~ Deprecate providesGVR changes and update validate script Sep 9, 2020

aiyengar2 changed the title ~~Deprecate providesGVR changes and update validate script~~ Deprecate providesGVR flag and update validate script Sep 9, 2020

cbron approved these changes Sep 9, 2020

View reviewed changes

dramich approved these changes Sep 9, 2020

View reviewed changes

brendarearden approved these changes Sep 9, 2020

View reviewed changes

aiyengar2 merged commit af76c27 into rancher:dev-v2.5 Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate providesGVR flag and update validate script #620

Deprecate providesGVR flag and update validate script #620

aiyengar2 commented Sep 9, 2020 •

edited

Loading

aiyengar2 Sep 9, 2020

mrajashree Sep 9, 2020 •

edited

Loading

dramich Sep 9, 2020

cbron Sep 9, 2020

aiyengar2 Sep 9, 2020 •

edited

Loading

cbron Sep 9, 2020

mrajashree left a comment

cbron left a comment

cbron Sep 9, 2020

aiyengar2 commented Sep 9, 2020 •

edited

Loading

cbron left a comment

Deprecate providesGVR flag and update validate script #620

Deprecate providesGVR flag and update validate script #620

Conversation

aiyengar2 commented Sep 9, 2020 • edited Loading

aiyengar2 Sep 9, 2020

Choose a reason for hiding this comment

mrajashree Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

dramich Sep 9, 2020

Choose a reason for hiding this comment

cbron Sep 9, 2020

Choose a reason for hiding this comment

aiyengar2 Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

cbron Sep 9, 2020

Choose a reason for hiding this comment

mrajashree left a comment

Choose a reason for hiding this comment

cbron left a comment

Choose a reason for hiding this comment

cbron Sep 9, 2020

Choose a reason for hiding this comment

aiyengar2 commented Sep 9, 2020 • edited Loading

cbron left a comment

Choose a reason for hiding this comment

aiyengar2 commented Sep 9, 2020 •

edited

Loading

mrajashree Sep 9, 2020 •

edited

Loading

aiyengar2 Sep 9, 2020 •

edited

Loading

aiyengar2 commented Sep 9, 2020 •

edited

Loading