Bring in `pkg/model/multiversion` package to help interacting with multiple API versions #121

a-hilaly · 2021-07-06T13:49:02Z

Part of aws-controllers-k8s/community#835

Description of changes:
This patch brings a new package to pkg/model, essentially to
simplify accessing and interacting with multiple api versions.

The pkg/model/multiversion contains five main components:

An APIVersionManager struct containing information about the hub
and spokes apis, their corresponding ackmodel.Model and
APIInfo, plus a list of the deprecated and removed versions.
A FieldDelta, CRDDelta and FiledChangeTypes which helps in
representing the difference between two different APIs.
A ComputeFieldsDiff function which helps in computing the difference
between two given string to ackmodel.Fields maps.
An APIStatus and APIInfo that holds information about a specific
api version (used sdk version, generator file path etc...)
Add ComputeRenamesDelta to compte the correct field renames
between two api versions.

this PRs also changes model.SDKHelper methods and signatures to
support interacting with multiple "Service API Versions" and multiple sdk
versions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly · 2021-07-06T13:50:46Z

NOTE to reviews: Please give this PR a manual merge.
This PR will need a rebase after #120.
/hold

RedbackThomson

Amazing work! Your implementation of the field diffs feels very well thought out and I walked through it without any confusion. I have a suggestion about introducing a metadata configuration file that might help solve the deprecation status (and other stuff) we have. This can be a separate issue that I'd be happy to tackle.

RedbackThomson · 2021-07-09T17:58:24Z

pkg/model/crd.go

+// GetAllRenames returns all the fields renames observed in the generator.yaml.
+// It will return two rename maps, the first is mapping the old names to the new
+// ones, the other is mapping the new names to the old ones.
+func (r *CRD) GetAllRenames() (
+	map[string]string,
+	map[string]string,
+	error,
+) {


I'm not sure how I feel about this return type. It feels a little dirty two have two structs with the same information. My immediate thought would be to have a list of structs that contains old and new, then use helper methods to determine old from new and vice versa.

type FieldRenames struct { renames []struct { old string new string } } func (r *FieldRenames) OldFromNew(string new) { ... } func (r *FieldRenames) NewFromOld(string old) { ... } ... renames := []FieldRename{...}

It's a bit more verbose, so maybe it's not necessary. Throwing this idea out there.

Actually, I would just return a single map[string]string representing the old name to new name.

Looking at this PR, the only place where the map of new to old name is used is in a test case, and @a-hilaly you can easily just reverse the map of old to new in a test utility method.

@jaypipes @RedbackThomson
I miss interpreted this problem when i started implementing ack-generate webhooks. The renames map should be computed separately, by comparing two rename maps (hub and spoke renames) and finding the "evolution" of renames from a source to destination API version.
For example if the renames didn't change from v1 to v2 our renames map should be empty.

I added ComputeRenamesDelta to properly compute the renames map in delta_renames.go

pkg/model/multiversion/api_info.go

pkg/model/multiversion/delta.go

pkg/model/multiversion/mapper.go

RedbackThomson · 2021-07-09T18:09:06Z

pkg/model/multiversion/mapper.go

+	deprecatedVersions []string
+	removedVersions    []string


I think we need a repository metadata configuration file. It should contain general metadata about the service - full name, short name, link to docs, etc. - but also a list of each version and their support state (deprecated, stable, under development). This would serve as the basis for the generated documentation as well.

Agree with @RedbackThomson that a metadata file about the repository would be useful. Not necessary to implement that in this PR, though. It could be a followup PR.

pkg/model/multiversion/api_info.go

pkg/model/multiversion/delta.go

jaypipes · 2021-07-09T18:59:03Z

pkg/model/multiversion/mapper.go

+	deprecatedVersions []string
+	removedVersions    []string


Agree with @RedbackThomson that a metadata file about the repository would be useful. Not necessary to implement that in this PR, though. It could be a followup PR.

pkg/model/multiversion/mapper.go

jaypipes · 2021-07-09T19:08:08Z

pkg/model/crd.go

+// GetAllRenames returns all the fields renames observed in the generator.yaml.
+// It will return two rename maps, the first is mapping the old names to the new
+// ones, the other is mapping the new names to the old ones.
+func (r *CRD) GetAllRenames() (
+	map[string]string,
+	map[string]string,
+	error,
+) {


Actually, I would just return a single map[string]string representing the old name to new name.

Looking at this PR, the only place where the map of new to old name is used is in a test case, and @a-hilaly you can easily just reverse the map of old to new in a test utility method.

pkg/model/multiversion/delta.go

jaypipes

@a-hilaly a few suggestions for renaming some variables and fields, otherwise this is very close to merging. Great stuff!

pkg/model/multiversion/api_info.go

pkg/model/multiversion/delta.go

pkg/model/multiversion/delta_test.go

pkg/model/multiversion/manager.go

pkg/model/sdk_helper.go

pkg/model/crd.go

RedbackThomson · 2021-07-12T23:19:20Z

pkg/model/crd.go

+
+	renames := make(map[string]string)
+	opMap := r.sdkAPI.GetOperationMap(r.cfg)
+	createOps := (*opMap)[OpTypeCreate]


Is this necessary? I understand it's future proofing, but either we should iterate through all operations or only iterate through create.

We should be only interested by CreateOp renames when using this function in multiversion.ComputeCRDDeltas. But that's a good point as well - i believe we should pass an op parameter to the function

pkg/model/crd.go

RedbackThomson · 2021-07-12T23:21:03Z

pkg/model/multiversion/api_info.go

+
+package multiversion
+
+// TODO(a-hilaly) move this file outside of pkg/model/multiversion. Idealy we


Why is this a TODO? Can we not do it in this PR?

This is very related to pkg/metadata that you're working on. Just keeping here and moving it to the right place when we'll have a good idea where it should be placed.

pkg/model/multiversion/delta_renames.go

RedbackThomson · 2021-07-12T23:35:54Z

pkg/model/multiversion/manager.go

+// APIVersionManager is a API versions manager. It contains the mapping
+// of each non-deprecated version with their correspending ackmodel.Model
+// and APIInfos.
+type APIVersionManager struct {


note to @RedbackThomson: Incorporate loading ServiceMetadata into APIVersionManager

RedbackThomson · 2021-07-12T23:37:26Z

pkg/model/multiversion/manager.go

+	// create model for each non-deprecated api version
+	models := make(map[string]*ackmodel.Model, len(apisInfo))
+	for apiVersion, apiInfo := range apisInfo {
+		// TODO(a-hilaly) filter deprecated and removed api versions


I think we will continue to support all released versions of the API? Why would we ever deprecate an API version? It costs us nothing to generate the webhooks.

We should continue to generate webhooks for Deprecated versions but not for the removed ones.
I think that at one point we should start deprecating versions and eventually remove them. I imagine that if one day we release a v1 api version - all alpha/beta version should be at least "Deprecated" if not removed

According to https://kubernetes.io/docs/reference/using-api/deprecation-policy/ we should not have more than one "Available" version at onces - and at most two "Deprecated" versions

RedbackThomson · 2021-07-12T23:40:02Z

pkg/model/multiversion/manager.go

+	gitRepo, err := util.LoadRepository(sdkCacheDir)
+	if err != nil {
+		return nil, fmt.Errorf("cannot read sdk git repository: %v", err)
+	}


Do we have tests that we can identify and load the repository correctly? All the delta tests use the same AWS SDK version

Do we have tests that we can identify and load the repository correctly?

Unfortunately no.. the testdata we have is a collection of text files and they are not part of an aws-sdk-go git repository.

All the delta tests use the same AWS SDK version

I just added new delta unit tests using multiple AWS SDK versions

RedbackThomson · 2021-07-12T23:42:20Z

pkg/testutil/schema_helper.go

+	// The AWS API version. Defaults to 00-00-0000
+	SDKAPIVersion string


nit: This is not the SDKAPIVersion, it's a versioned date string related to the API backend format (or something like that). It should not be confused for the AWS SDK version.

Yepp changed to ServiceAPIVersion to avoid confusion with AWS SDK Version.

In the api-2.json it is called apiVersion https://github.com/aws/aws-sdk-go/blob/main/models/apis/kafka/2018-11-14/api-2.json#L3

a-hilaly · 2021-07-13T12:38:02Z

Current tests total coverage: 54.8%

go test -coverprofile test.out -tags codegen -v ./pkg/model/multiversion/...
go tool cover -func test.out

github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta.go:71:		ComputeCRDFieldsDeltas	68.8%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta.go:104:		fieldChangedToSecret	100.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta.go:112:		ComputeFieldsDelta	95.7%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta.go:251:		getFieldNameFromDelta	100.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta.go:261:		AreEqualShapes		66.7%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/delta_renames.go:36:	ComputeRenamesDelta	100.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:51:		NewAPIVersionManager	0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:113:		GetModel		0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:121:		GetSpokeVersions	0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:126:		GetHubVersion		0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:132:		CompareHubWith		0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:137:		VerifyAPIVersions	0.0%
github.com/aws-controllers-k8s/code-generator/pkg/model/multiversion/manager.go:155:		CompareAPIVersions	0.0%
total:												(statements)		54.8%

The total coverage is low because multiversion.Manager doesn't have any tests. To add tests for it we'll need a way to mock git.CheckoutRepository. @jaypipes @vijtrip2 @RedbackThomson if you think that it is necessary i can add it in a follow up PR.

jaypipes

minor variable rename suggestions and typos, but the code looks good, thank you Amine!

pkg/model/multiversion/delta.go

pkg/model/sdk_helper.go

…per` This patch adds a new methods to `ackmodel.SDKHelper` which will help in loading api files for different aws-sdk-go versions and different service api versions.

…yaml` This patch introduces a new function that returns all the observed renames for a given CRD and for a specific `generator.yaml` file. It returns two different maps, the first mapping old names with new ones and the other mapping old ones with new ones.

multiple api versions. This patch brings a new package to `pkg/model`, essentially to simplify accessing and interacting with multiple api versions. The `pkg/model/multiversion` contains four main components: - An `APIVersionManager` struct which is a mapper containing information about the hub and spokes apis, their corresponding `ackmodel.Model` and `APIInfo`, plus a list of the deprecated and removed versions. - A `FieldDelta`, `CRDDelta` and `FiledChangeTypes` which helps in representing the difference between two different APIs. - A `ComputeFieldsDiff` function which helps in computing the difference between two given string to `ackmodel.Fields` maps. - An `APIStatus` and `APIInfo` that holds information about a specific api version (used sdk version, generator file path etc...) - Add `computeRenamesDelta` to detect the correct field renames from a version to another

jaypipes

👍

jaypipes · 2021-07-13T14:20:44Z

/lgtm

ack-bot · 2021-07-13T14:20:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,jaypipes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-bot requested review from jaypipes and vijtrip2 July 6, 2021 13:49

ack-bot added the approved label Jul 6, 2021

ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2021

a-hilaly mentioned this pull request Jul 6, 2021

Refactor pkg/generate package #120

Merged

a-hilaly force-pushed the multiversion-package branch from c60807a to 68d9648 Compare July 6, 2021 13:57

a-hilaly mentioned this pull request Jul 6, 2021

Introduce ack-generate webhooks sub command #122

Closed

a-hilaly force-pushed the multiversion-package branch 2 times, most recently from 220c2e2 to 9b2d23b Compare July 9, 2021 16:47

a-hilaly changed the title ~~Bring in pkg/generate/multiversion package to help interacting with multiple API versions~~ Bring in pkg/model/multiversion package to help interacting with multiple API versions Jul 9, 2021

a-hilaly force-pushed the multiversion-package branch from 9b2d23b to b09287b Compare July 9, 2021 16:48

RedbackThomson reviewed Jul 9, 2021

View reviewed changes

vijtrip2 reviewed Jul 9, 2021

View reviewed changes

jaypipes requested changes Jul 9, 2021

View reviewed changes

a-hilaly force-pushed the multiversion-package branch 5 times, most recently from 83189cf to a20ca69 Compare July 12, 2021 14:08

jaypipes requested changes Jul 12, 2021

View reviewed changes

a-hilaly force-pushed the multiversion-package branch 6 times, most recently from 808d78f to 8c12960 Compare July 12, 2021 21:09

RedbackThomson reviewed Jul 12, 2021

View reviewed changes

a-hilaly force-pushed the multiversion-package branch 2 times, most recently from 989df64 to 19dd2d3 Compare July 13, 2021 11:22

a-hilaly force-pushed the multiversion-package branch from 19dd2d3 to be1a648 Compare July 13, 2021 11:59

a-hilaly requested review from RedbackThomson, jaypipes and vijtrip2 July 13, 2021 12:02

a-hilaly force-pushed the multiversion-package branch 2 times, most recently from 2ad7f51 to 0fb3cb7 Compare July 13, 2021 13:43

jaypipes approved these changes Jul 13, 2021

View reviewed changes

a-hilaly added 3 commits July 13, 2021 16:13

Add WithSDKVersion and WithAPIVersion methods to `ackmodel.SDKHel…

43b45a0

…per` This patch adds a new methods to `ackmodel.SDKHelper` which will help in loading api files for different aws-sdk-go versions and different service api versions.

a-hilaly force-pushed the multiversion-package branch from 0fb3cb7 to 58764f2 Compare July 13, 2021 14:15

jaypipes approved these changes Jul 13, 2021

View reviewed changes

ack-bot assigned jaypipes Jul 13, 2021

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021

jaypipes merged commit 9aa3fb4 into aws-controllers-k8s:main Jul 13, 2021

jaypipes removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2021


		package multiversion

		// TODO(a-hilaly) move this file outside of pkg/model/multiversion. Idealy we

		// The AWS API version. Defaults to 00-00-0000
		SDKAPIVersion string

Bring in pkg/model/multiversion package to help interacting with multiple API versions #121

Bring in pkg/model/multiversion package to help interacting with multiple API versions #121

Uh oh!

Conversation

a-hilaly commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaypipes left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jul 13, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bring in `pkg/model/multiversion` package to help interacting with multiple API versions #121

Bring in `pkg/model/multiversion` package to help interacting with multiple API versions #121

a-hilaly commented Jul 6, 2021 •

edited

Loading

a-hilaly commented Jul 6, 2021 •

edited

Loading

a-hilaly Jul 13, 2021 •

edited

Loading

a-hilaly commented Jul 13, 2021 •

edited

Loading