Add service metadata configuration file #126

RedbackThomson · 2021-07-13T21:51:20Z

Description of changes:
Creates a new specification for a metadata.yaml file that resides in the root of the controller repository. The purpose of this file is to provide human-friendly display information about the service (long name, acronym, links) and to provide a source of truth for API versions.

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

pkg/metadata/service_metadata.go

jaypipes · 2021-07-13T23:19:02Z

go.mod

 	github.com/spf13/cobra v1.1.1
 	github.com/stretchr/testify v1.7.0
 	golang.org/x/mod v0.4.1
+	golang.org/x/tools v0.0.0-20200616195046-dc31b401abb5


where'd this come from? maybe remove it with go mod tidy?

jaypipes · 2021-07-13T23:23:05Z

pkg/metadata/api_info.go

-	APIStatusRemoved              = "removed"
-	APIStatusDeprecated           = "deprecated"
+	APIStatusUnknown       APIStatus = "unknown"
+	APIStatusInDevelopment           = "in_development"


Can we call this just development and make it the default instead of unknown?

I'm wondering if we really need a development status for API versions. I think that we should increase the API Version whenever the CRD schema changes.

IMO development should be a status for the service controller and not a specific API version. Unless, if development means "not released" with the controller helm chart yet.

That seems fair. The API version in development will always be the latest available option. "In development" vs. "released" is controlled by our controller release already.

jaypipes · 2021-07-13T23:26:18Z

pkg/testdata/models/metadata.yaml

+  documentation: https://docs.example.com/
+versions:
+- api_version: v1alpha1
+  status: development


It might be useful to have a helper script in the code-generator that will examine a new controller repository and construct a default metadata file given a service name...

I'm curious to know what would happen if we don't have an APIStatusDevelopment

a-hilaly · 2021-07-14T10:14:24Z

pkg/metadata/api_info.go

-	APIStatusRemoved              = "removed"
-	APIStatusDeprecated           = "deprecated"
+	APIStatusUnknown       APIStatus = "unknown"
+	APIStatusInDevelopment           = "in_development"


I'm wondering if we really need a development status for API versions. I think that we should increase the API Version whenever the CRD schema changes.

a-hilaly · 2021-07-14T10:20:25Z

pkg/metadata/api_info.go

-	APIStatusRemoved              = "removed"
-	APIStatusDeprecated           = "deprecated"
+	APIStatusUnknown       APIStatus = "unknown"
+	APIStatusInDevelopment           = "in_development"


IMO development should be a status for the service controller and not a specific API version. Unless, if development means "not released" with the controller helm chart yet.

pkg/metadata/service_metadata.go

a-hilaly · 2021-07-14T10:30:59Z

pkg/metadata/service_metadata.go

+type ServiceVersion struct {
+	APIVersion string    `json:"api_version"`
+	Status     APIStatus `json:"status"`
+}


We could also add a boolean field called IsHub, to be able to override the hub version. In most of the cases the latest Available version is the hub (prefered storage version). However, according to k8s deprecation policy sometimes the hub can be different from the latest version.

I am quite averse to the idea of the hub being anything other than the latest generated version - we rely on it being the latest for the common runtime elements to match.

a-hilaly · 2021-07-14T10:33:45Z

pkg/model/model.go

+	// Metadata for the service
+	meta *ackmetadata.ServiceMetadata


Do we need *ackmetadata.ServiceMetadata in ackmode.Model and multiversion.APIVersionManager? what are the use cases when generating one single api version (ack-generate apis)?

True. I had this in before your last PR, I'll remove it again.

a-hilaly · 2021-07-14T10:37:01Z

pkg/model/multiversion/manager.go

 		apiInfos:      apisInfo,
 		models:        models,
 	}
-	// TODO(hilalymh): Audit deprecated and removed versions


I'm still going to send a PR for API versions examination. Mainly to verify that service controller maintainers didn't violate one of the deprecation/removal policies.

a-hilaly · 2021-07-14T10:40:28Z

pkg/model/multiversion/manager.go

+	hubVersion, err := metadata.GetLatestAPIVersion()
+	if err != nil {
+		return nil, err
+	}


The hub version is not always the latest version. I would prefer to keep the APIVersionManager flexible in term of choosing a hub version. However we can still enforce "choosing the latest version as hub" in the build script or cmd / pkg/generate/ack

Sounds good. Will wait for your changes to cmd - we can load it in there.

pkg/model/multiversion/manager.go

a-hilaly · 2021-07-14T10:44:19Z

pkg/metadata/service_metadata.go

+// path to a metadata file
+func NewServiceMetadata(
+	metadataPath string,
+) (ServiceMetadata, error) {


nit: you can return a *ServiceMetadata to avoid allocating a new ServiceMetadata{} when you return an error

a-hilaly

Looks good overall! 👍

scripts/lib/metadata.sh

a-hilaly · 2021-07-14T17:52:56Z

pkg/testdata/models/metadata.yaml

+  documentation: https://docs.example.com/
+versions:
+- api_version: v1alpha1
+  status: development


I'm curious to know what would happen if we don't have an APIStatusDevelopment

a-hilaly · 2021-07-14T18:02:45Z

pkg/metadata/service_metadata.go

+func (m *ServiceMetadata) GetLatestAPIVersion() (string, error) {
+	availableVersions := m.GetAvailableAPIVersions()
+
+	if len(availableVersions) == 0 {
+		return "", ErrNoAvailableVersions
+	}
+
+	return availableVersions[len(availableVersions)-1], nil
+}
+
+func (m *ServiceMetadata) GetDeprecatedAPIVersions() []string {
+	return m.getVersionsByStatus(APIStatusDeprecated)
+}
+
+func (m *ServiceMetadata) GetRemovedAPIVersions() []string {
+	return m.getVersionsByStatus(APIStatusRemoved)
+}
+
+func (m *ServiceMetadata) GetAvailableAPIVersions() []string {
+	return m.getVersionsByStatus(APIStatusAvailable)
+}


half nit:please add godoc comment, at least for the exported functions

jaypipes

Nice work on this @RedbackThomson :)

jaypipes · 2021-07-16T13:40:31Z

scripts/construct-metadata.sh

+DEFAULT_METADATA_OUTPUT_PATH="$SERVICE_CONTROLLER_SOURCE_PATH/metadata.yaml"
+METADATA_OUTPUT_PATH="${METADATA_OUTPUT_PATH:-$DEFAULT_METADATA_OUTPUT_PATH}"
+
+echo "🧙 Welcome to the service metadata setup wizard"


sweet emojis.

jaypipes · 2021-07-16T13:41:29Z

/lgtm

a-hilaly · 2021-07-16T13:45:20Z

/lgtm

ack-bot · 2021-07-16T13:45:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,RedbackThomson,jaypipes]

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

Nicholas Thomson added 5 commits July 13, 2021 14:06

Add initial metadata struct

bc244b1

Move generation metadata

3fe7227

Add GetDevelopmentAPIVersion helper

94b8b7b

Rebase on APIStatus

312504d

Load metadata in multiversion manager

ecd2144

RedbackThomson requested review from a-hilaly and jaypipes July 13, 2021 21:51

ack-bot requested a review from vijtrip2 July 13, 2021 21:51

ack-bot added the approved label Jul 13, 2021

jaypipes requested changes Jul 13, 2021

View reviewed changes

RedbackThomson changed the title ~~Service metadata configuration file~~ Add service metadata configuration file Jul 13, 2021

Define errors as vars

21380e1

a-hilaly reviewed Jul 14, 2021

View reviewed changes

Nicholas Thomson added 3 commits July 14, 2021 10:33

Remove metadata from model

1dad7ad

Remove development API status

f5c9d83

Go mod tidy

5f5a820

a-hilaly reviewed Jul 14, 2021

View reviewed changes

Nicholas Thomson added 2 commits July 14, 2021 11:13

Add metadata construct script

7967740

Add GoDocs to metadata fields

0e4c103

a-hilaly approved these changes Jul 15, 2021

View reviewed changes

jaypipes approved these changes Jul 16, 2021

View reviewed changes

ack-bot assigned jaypipes Jul 16, 2021

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

ack-bot assigned a-hilaly Jul 16, 2021

ack-bot merged commit 75e6c92 into aws-controllers-k8s:main Jul 16, 2021

a-hilaly mentioned this pull request Jul 16, 2021

Introduce ack-generate webhooks sub command #122

Closed

		// Metadata for the service
		meta *ackmetadata.ServiceMetadata

Add service metadata configuration file #126

Add service metadata configuration file #126

Uh oh!

Conversation

RedbackThomson commented Jul 13, 2021

Uh oh!

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!

a-hilaly Jul 14, 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!

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!

a-hilaly Jul 14, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jul 14, 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!

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jul 14, 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!

a-hilaly Jul 14, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

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!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Jul 16, 2021

Uh oh!

a-hilaly commented Jul 16, 2021

Uh oh!

ack-bot commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

a-hilaly Jul 14, 2021 •

edited

Loading

a-hilaly Jul 14, 2021 •

edited

Loading

a-hilaly Jul 14, 2021 •

edited

Loading

a-hilaly Jul 14, 2021 •

edited

Loading

a-hilaly Jul 14, 2021 •

edited

Loading