generate: parse CSV annotations with `controller-tools/pkg/markers` by estroz · Pull Request #2792 · operator-framework/operator-sdk

estroz · 2020-04-07T07:35:33Z

Description of the change: use sigs.k8s.io/controller-tools/pkg/markers to re-write the CSV annotations (now known as markers) machinery.

Motivation for the change: speedup of ~20x based on unit test runtime:

# Uses new markers.
$ go test ./internal/generate/clusterserviceversion/...
ok  	github.com/operator-framework/operator-sdk/internal/generate/clusterserviceversion	1.683s
ok  	github.com/operator-framework/operator-sdk/internal/generate/clusterserviceversion/bases	(cached)
ok  	github.com/operator-framework/operator-sdk/internal/generate/clusterserviceversion/bases/definitions	0.898s

# Uses old annotations.
$ go test ./internal/generate/olm-catalog/...
ok  	github.com/operator-framework/operator-sdk/internal/generate/olm-catalog	86.300s
ok  	github.com/operator-framework/operator-sdk/internal/generate/olm-catalog/descriptor	64.433s

TODO:

make sure we're ok with breaking changes to marker formats (there's no good way to maintain the old format with these changes)
clean up a few things left over from testing.

joelanford · 2020-04-14T13:27:33Z

What is the possibility of introducing the new format while simultaneously keeping the old format, so that we can deprecate without immediately breaking and give folks time to transition?

estroz · 2020-04-15T00:45:19Z

@joelanford that may prove difficult but let me see how easy it is first.

Edit: the dot-in-path (ex. customresourcedefinitions.displayName) is not allowed by the markers system. As far as I can tell we have two choices:

Make a breaking change now.
Support the new format using gengo parsing (the current method), and break later.

While method 2 does provide a true deprecation path, it will require some extra work to parse the new marker format.

estroz · 2020-04-17T22:34:54Z

Two more options:

Write a migration script (effectively sed -i 's/old format/new format/g') which would be fairly easy.
Keep all the old gengo code/format for legacy projects, and advertise the new format for kubebuilder projects.

joelanford · 2020-04-21T14:53:17Z

@estroz

Two more options:

Write a migration script (effectively sed -i 's/old format/new format/g') which would be fairly easy.

Keep all the old gengo code/format for legacy projects, and advertise the new format for kubebuilder projects.

Can we do both of these and also deprecate the old gengo code/format for legacy projects?

So the result would be:

Kubebuilder projects are only compatible with the new format
Legacy projects are compatible with both formats for v0.18.0 and v0.19.0
Starting with v0.20.0, legacy project are only compatible with the new format

This way we have a path to fully removing the old gengo-based parsing.

estroz · 2020-04-28T17:49:56Z

@joelanford done adding back old behavior, take a look. Will write a migration script and docs.

estroz · 2020-06-23T21:36:33Z

/ping @hasbro17 @joelanford @jmrodri @jmccormick2001 @camilamacedo86

camilamacedo86

@estroz,

I tested it with a personal project and could not find any issue. So, all shows fine when has not the PROJECT file.

After, just add the project file I started to face :

$ operator-sdk generate csv --csv-version 0.0.2 --update-crds FATA[0000] version format must match ^(v)?([1-9][0-9]*)(-alpha|-beta)?$

Also, could you please clarify better in the first comment what are the breaking changes? Are we changing the markers?
I am not sure about what changes with this PR, would be nice have this info in the first comment for we know what exactly we need test and verify. Also, the motivation here just makes it runs faster?

estroz · 2020-07-13T22:46:39Z

Closes #2983

tlwu2013

Hi @estroz , this looks great! Spot a few errors from the example in the doc, can you help take a look? thanks!

tlwu2013 · 2020-07-14T02:35:03Z

+	    - description: The desired number of member Pods for the deployment.
+	      displayName: Size
+	      path: podStatuses.size
+	    - displayName: Size


The description is a required property of both specDescriptors and statusDescriptors. We may need to add the default data for description field instead of omitting it.

Just checked the struct again and notice in fact only the path is the required property. ~~So no concerns on the examples as well.~~

I almost regret immediately after posting my previous comments. :(

Although "technically" only path is the required property, an empty description leads to an empty tooltip on the UI:

Tooltip - with description:

Empty tooltip - without description:

Can we at least include some simple text for the description field in the example (UI will be adding better handling for the empty description case)? Thanks!

This is something I'd like to leave up to the OperatorHubValidator, since it is perfectly valid to have a CSV without these descriptions.

joelanford

LGTM. Just a few nits/questions.

joelanford · 2020-07-21T23:45:07Z

+			if kbutil.HasProjectFile() || areAnnotationsMigrated(b.APIsDir) {
+				err = updateDefinitions(base, b.APIsDir, b.GVKs)
+			} else {
+				err = updateDescriptionsForGVKs(base, b.APIsDir, b.GVKs)
+			}


We require that projects have a PROJECT file now, right? So no need for the updateDescriptionsForGVKs() call?

Also, as @fabianvf was suggesting for generate crds here, would it make sense to remove the requirement for projutil.OperatorTypeGo and only require that there are some parseable .go files?

Shouldn't that requirement check be centralized in IsOperatorGo()?

hack/generate/migrate-markers.sh: annotation -> marker migration script internal/generate/olm-catalog: use legacy descriptor parsing if neither PROJECT nor the new markers format exists

…y incorrect name

jmrodri

/lgtm
I didn't see any red flags. Good job.

jmrodri · 2020-07-22T17:47:30Z

+  local old_marker_pattern_1="${OLD_MARKER_BASE}"'\.resources="([^\,]+)\,([^\,]+)\,\\"([^\\]+)\\""'
+  local old_marker_pattern_2="${OLD_MARKER_BASE}"'\.resources="([^\,]+)\,([^\,]+)\,"'
+  local new_marker_pattern_1="${NEW_MARKER_BASE}"':resources={\1,\2,"\3"}'
+  local new_marker_pattern_2="${NEW_MARKER_BASE}"':resources={\1,\2,}'
+


Impressive! regex always impresses me :)

openshift-ci-robot · 2020-07-22T19:05:15Z

New changes are detected. LGTM label has been removed.

estroz added the olm-integration Issue relates to the OLM integration label Apr 7, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2020

openshift-ci-robot requested review from jmrodri and varshaprasad96 April 7, 2020 07:35

estroz requested review from hasbro17 and joelanford April 7, 2020 07:42

camilamacedo86 reviewed Apr 7, 2020

View reviewed changes

Comment thread internal/generate/olm-catalog/csv_updaters.go Outdated

camilamacedo86 reviewed Apr 7, 2020

View reviewed changes

Comment thread internal/generate/olm-catalog/descriptor/crd.go Outdated

camilamacedo86 reviewed Apr 7, 2020

View reviewed changes

Comment thread internal/generate/olm-catalog/descriptor/parse_test.go

camilamacedo86 reviewed Apr 7, 2020

View reviewed changes

Comment thread internal/generate/testdata/go/pkg/apis/cache/v1alpha1/dummy_types.go Outdated

estroz force-pushed the refactor/csv-markers branch 2 times, most recently from 88cd734 to a53c74a Compare April 7, 2020 22:53

estroz marked this pull request as ready for review April 14, 2020 01:04

estroz removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2020

estroz force-pushed the refactor/csv-markers branch from a53c74a to c5d0103 Compare April 15, 2020 00:50

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020

estroz force-pushed the refactor/csv-markers branch from c5d0103 to 9446856 Compare April 17, 2020 21:42

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2020

estroz force-pushed the refactor/csv-markers branch from 9446856 to 7cd3e7a Compare April 24, 2020 17:32

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2020

estroz force-pushed the refactor/csv-markers branch 3 times, most recently from a7ed23d to 064aeda Compare April 28, 2020 17:43

camilamacedo86 reviewed Apr 29, 2020

View reviewed changes

Comment thread internal/generate/olm-catalog/csv.go Outdated

estroz mentioned this pull request May 8, 2020

Need a way to disable path parse for gen csv #2983

Closed

estroz force-pushed the refactor/csv-markers branch 3 times, most recently from c50ef88 to d0f4666 Compare May 11, 2020 19:33

estroz force-pushed the refactor/csv-markers branch 2 times, most recently from 1480b36 to 6095653 Compare June 23, 2020 21:07

estroz force-pushed the refactor/csv-markers branch from 6095653 to 84b26eb Compare June 23, 2020 21:54

estroz added this to the v1.0.0 milestone Jul 2, 2020

joelanford mentioned this pull request Jul 2, 2020

Operator SDK 1.0 Meta-issue #3327

Closed

92 tasks

camilamacedo86 reviewed Jul 6, 2020

View reviewed changes

estroz force-pushed the refactor/csv-markers branch from 84b26eb to d4b5758 Compare July 13, 2020 21:33

estroz mentioned this pull request Jul 14, 2020

Remove legacy generate and bundle commands #3414

Merged

5 tasks

tlwu2013 reviewed Jul 14, 2020

View reviewed changes

joelanford approved these changes Jul 22, 2020

View reviewed changes

Eric Stroczynski added 2 commits July 22, 2020 10:39

use controller-tools/pkg/markers to parse CSV annotations in types

36ec065

hack/generate/migrate-markers.sh: annotation -> marker migration script internal/generate/olm-catalog: use legacy descriptor parsing if neither PROJECT nor the new markers format exists

remove legacy markers

a2a1993

estroz force-pushed the refactor/csv-markers branch from d4b5758 to bb31df0 Compare July 22, 2020 18:35

use GVKs directly instead of creating definition keys with potentiall…

598997b

…y incorrect name

estroz force-pushed the refactor/csv-markers branch from bb31df0 to 598997b Compare July 22, 2020 18:58

jmrodri approved these changes Jul 22, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Jul 22, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020

remove more unused code

6ee7092

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020

estroz changed the title ~~generate csv: parse CSV annotations with controller-tools/pkg/markers~~ generate: parse CSV annotations with controller-tools/pkg/markers Jul 22, 2020

estroz merged commit 5256f82 into operator-framework:master Jul 22, 2020

estroz deleted the refactor/csv-markers branch July 22, 2020 19:48

Conversation

estroz commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joelanford commented Apr 14, 2020

Uh oh!

estroz commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz commented Apr 17, 2020

Uh oh!

joelanford commented Apr 21, 2020

Uh oh!

estroz commented Apr 28, 2020

Uh oh!

Uh oh!

estroz commented Jun 23, 2020

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz commented Jul 13, 2020

Uh oh!

tlwu2013 left a comment

Choose a reason for hiding this comment

Uh oh!

tlwu2013 Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

tlwu2013 Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlwu2013 Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estroz Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

joelanford Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

estroz Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

jmrodri Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

estroz commented Apr 7, 2020 •

edited

Loading

estroz commented Apr 15, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

tlwu2013 Jul 21, 2020 •

edited

Loading

tlwu2013 Jul 21, 2020 •

edited

Loading

estroz Jul 22, 2020 •

edited

Loading