Implement KEP-19 by distinguishing between app and operator version #1257

nfnt · 2020-01-09T09:20:56Z

What this PR does / why we need it:
The app version of an operator package is taken into account using operations that apply to packages. E.g., when installing a package, a user is able to provide a specific app version to install. To further distinguish app version and operator version, the 'version' field in operator packages and the package index has been renamed to 'operatorVersion'.
Package version resolution has been updated to take the app version into account; operator packages are first sorted by app version, then sorted by operator version. Furthermore, the app version is added to the package tarball name to avoid conflicts.

Notes for reviewers:
Most of the changes update the existing tests and their test data. Functional changes are:

Update CLI commands to have app-version and operator-version flags
Update package resolvers to take app version into account
Update the tarball writer and repo index to include app version in tarball names
Update the PackageVersions struct to be sorted first by app version, then by operator version

The app version of an operator package is taken into account using operations that apply to packages. E.g., when installing a package, a user is able to provide a specific app version to install. To further distinguish app version and operator version, the 'version' field in operator packages and the package index has been renamed to 'operatorVersion'. Package version resolution has been updated to take the app version into account; operator packages are first sorted by app version, then sorted by operator version. Furthermore, the app version is added to the package tarball name to avoid conflicts.

nfnt · 2020-01-09T09:49:05Z

The part of the E2E test that tests the current operators repository is expected to fail because the repository needs to be updated to reflect that version has been renamed to operatorVersion.

zen-dog

Aside from a few nits, my only point would be using _ vs. - in url/tarbal names. Though not strictly necessary, we should make our lives easier and use _ since - can be part of SemVer.

pkg/kudoctl/cmd/package_list_plans.go

pkg/kudoctl/cmd/upgrade.go

pkg/kudoctl/packages/resolver/resolver.go

pkg/kudoctl/packages/writer/writer_tar.go

pkg/kudoctl/util/repo/index.go

Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>

zen-dog

Lgtm!

  ________
< Ship it! >
  --------
         \   ^__^ 
          \  (oo)\_______
             (__)\       )\/\
                 ||----w |
                 ||     ||

kensipe

we have to fix the issue with - vs _ before it this can be merged

pkg/kudoctl/packages/writer/writer_tar.go

kensipe · 2020-01-09T22:05:19Z

pkg/kudoctl/util/repo/index.go

+		return versionX.LessThan(versionY)
+	}
+
+	return appVersionX.LessThan(appVersionY)


I feel we should have a number of tests around this. This is more complex than it was originally.

I still feel this is true... but approving with the assumption that it will happen or will have an issue created to do it.

I added some additional test cases to TestParseIndexFile in index_test.go. ~~But looking at it again, it doesn't test this particular code path. I'll add more entries to the test index to make sure that every case is covered.~~ Looked at it again, all cases are covered, this particular line it tested by

assert.Equal(t, "2.3.0", index.Entries["kafka"][0].AppVersion, "kafka app version") assert.Equal(t, "0.2.0", index.Entries["kafka"][0].OperatorVersion, "kafka operator version") assert.Equal(t, "2.2.1", index.Entries["kafka"][1].AppVersion, "kafka app version") assert.Equal(t, "0.2.0", index.Entries["kafka"][1].OperatorVersion, "kafka operator version")

in TestParseIndexFile.

kensipe · 2020-01-09T22:10:25Z

locally TestAddContainers failed in integration-tests

kensipe · 2020-01-09T22:30:44Z

this resulted in a package named Package created: /Users/kensipe/repo/cassandra-3.11.4_.tgz :(

kensipe · 2020-01-09T22:32:05Z

it's the operators fault! on it.

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe

looks good in general.. nice work!

I would love to see more tests around packageversion equality.

Created new 0.10.0 repo... not all the operators serialize. We need to talk about what will be in the repo. It is understandable that older versions with older formats remain (in that long term that is what we want). however the older kudo can't read the newer index and the new kudo can read the older operator formats. They need to be pruned.

List of what didn't make it:

flink 0.1.0
flink-demo 0.1.1
kafka 0.1.0, 0.1.2, 0.2.0
zookeeper 0.1.0

What is there needs to have the operator version incremented... but as mentioned, the older version doesn't make sense to continue to include IMO. We can talk about it.

new repo: https://kudo-repository.storage.googleapis.com/0.10.0

nfnt · 2020-01-10T12:44:05Z

Thanks @kensipe for taking care of providing a new repository!
Is there a branch for this in the operators repo? Asking because the e2e tests currently check out master of operators and tests the operators. As the operator definitions aren't compatible with the changes of this PR, the E2E tests currently fail. Do you plan to update the operators repo once this PR has landed?

kensipe · 2020-01-10T16:50:03Z

e2e test failing due to out of sync operator repo. fixed with kudobuilder/operators#184

rerunning tests after merge

…1257) The app version of an operator package is taken into account using operations that apply to packages. E.g., when installing a package, a user is able to provide a specific app version to install. To further distinguish app version and operator version, the 'version' field in operator packages and the package index has been renamed to 'operatorVersion'. Package version resolution has been updated to take the app version into account; operator packages are first sorted by app version, then sorted by operator version. Furthermore, the app version is added to the package tarball name to avoid conflicts. Co-authored-by: Aleksey Dukhovniy <adukhovniy@mesosphere.io> Co-authored-by: Ken Sipe <kensipe@gmail.com> Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt added the kep/19 label Jan 9, 2020

nfnt self-assigned this Jan 9, 2020

Update harness operators

d8b8ad5

nfnt marked this pull request as ready for review January 9, 2020 09:46

nfnt requested review from alenkacz, gerred, kensipe and zen-dog as code owners January 9, 2020 09:46

nfnt requested review from ANeumann82, mpereira and zmalik January 9, 2020 09:47

nfnt mentioned this pull request Jan 9, 2020

Update docs to reflect versioning changes kudobuilder/kudo.dev#124

Merged

zen-dog requested changes Jan 9, 2020

View reviewed changes

Jan Schlicht and others added 2 commits January 9, 2020 12:31

Apply suggestions from code review

5ea55bf

Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>

Keep changes consistent

0cd3423

nfnt requested a review from zen-dog January 9, 2020 11:50

zen-dog approved these changes Jan 9, 2020

View reviewed changes

kensipe requested changes Jan 9, 2020

View reviewed changes

switching to new 0.10.0 repo

3607c7f

Signed-off-by: Ken Sipe <kensipe@gmail.com>

kensipe approved these changes Jan 10, 2020

View reviewed changes

Use 0.10 repo in golden files

494dbc6

nfnt force-pushed the nfnt/kep19-impl branch 2 times, most recently from 13f496f to 494dbc6 Compare January 10, 2020 16:11

kensipe merged commit d2310b1 into master Jan 10, 2020

kensipe deleted the nfnt/kep19-impl branch January 10, 2020 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement KEP-19 by distinguishing between app and operator version #1257

Implement KEP-19 by distinguishing between app and operator version #1257

nfnt commented Jan 9, 2020 •

edited

Loading

nfnt commented Jan 9, 2020

zen-dog left a comment

zen-dog left a comment

kensipe left a comment

kensipe Jan 9, 2020

kensipe Jan 10, 2020

nfnt Jan 10, 2020 •

edited

Loading

kensipe commented Jan 9, 2020

kensipe commented Jan 9, 2020

kensipe commented Jan 9, 2020

kensipe left a comment •

edited

Loading

nfnt commented Jan 10, 2020

kensipe commented Jan 10, 2020

Implement KEP-19 by distinguishing between app and operator version #1257

Implement KEP-19 by distinguishing between app and operator version #1257

Conversation

nfnt commented Jan 9, 2020 • edited Loading

nfnt commented Jan 9, 2020

zen-dog left a comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Jan 9, 2020

Choose a reason for hiding this comment

kensipe Jan 10, 2020

Choose a reason for hiding this comment

nfnt Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

kensipe commented Jan 9, 2020

kensipe commented Jan 9, 2020

kensipe commented Jan 9, 2020

kensipe left a comment • edited Loading

Choose a reason for hiding this comment

nfnt commented Jan 10, 2020

kensipe commented Jan 10, 2020

nfnt commented Jan 9, 2020 •

edited

Loading

nfnt Jan 10, 2020 •

edited

Loading

kensipe left a comment •

edited

Loading