API Skeleton 2021-01-31 - swagger #1200

mjudeikis · 2020-11-27T09:59:48Z

This is just a skeleton builds on top of #1191

This PR does:

enables us to generate swagger for multiple apis in more repeatable way based on features apis delivers
Adds placeholders for some new api fields we agreed to add (SDN provider, kubeconfig download endpoint, encryption at host, more VM types (enum adding)). No feature behind them yet.
features will be implemented later

jim-minter

This is looking pretty plausible to me. Let's catch up on this tomorrow.

pkg/api/v20210131preview/openshiftclustercredentials.go

github-actions · 2020-12-04T17:53:17Z

Please rebase pull request.

jim-minter

just a lot of nits - please fix out and we'll merge.

Converters and changes to internal API are missing here - OK with those being in a follow-up PR.

pkg/api/v20210131preview/openshiftadmincredentials.go

pkg/api/v20210131preview/openshiftadmincredentials_example.go

pkg/swagger/swagger.go

pkg/swagger/typewalker.go

pkg/util/azureclient/mgmt/redhatopenshift/2021-01-31-preview/redhatopenshift/operations.go

mjudeikis · 2020-12-18T09:11:44Z

Azure/azure-rest-api-specs#12144 upstream review

mjudeikis · 2021-01-04T14:48:23Z

We had an issue with systemData.
https://github.com/Azure/azure-rest-api-specs/pull/12144/checks?check_run_id=1564328520
https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/common-api-contracts.md#system-metadata-for-all-azure-resources

I interpreted this as a mandatory field on the main object we return. Hence adding it to the OpenShiftCluster object only.

In addition, added struct wrapper to manage API features in all the generators.

mjudeikis · 2021-01-04T14:52:26Z

@jim-minter, the last commit will require your attention.

pkg/api/v20210131preview/openshiftclusteradminkubeconfig_example.go

pkg/swagger/examples.go

pkg/swagger/swagger.go

jim-minter · 2021-02-10T18:54:23Z

pkg/swagger/generator.go

+var _ generator = &apiv20200430{}
+
+type features struct {
+	kubeconfig         bool


not needed - test if ExampleOpenShiftClusterAdminKubeconfigResponse is non-nil?

Easier to read and easier to understand. As much as it is possible I think consistency is better if we keep same behavior (feature flags). Otherwise some of them will be "flags, some function checks". Unreadable and insiders knowledge.

jim-minter · 2021-02-10T20:10:10Z

pkg/swagger/generator.go

+	systemData         bool
+}
+
+type apiv20210131preview struct {


I think this is pretty over engineered - you could probably simplify and get away with a static configuration like this:

type api struct { exampleOpenShiftClusterPutParameter func() interface{} exampleOpenShiftClusterPatchParameter func() interface{} exampleOpenShiftClusterResponse func() interface{} exampleOpenShiftClusterCredentialsResponse func() interface{} exampleOpenShiftClusterAdminKubeconfigResponse func() interface{} exampleOpenShiftClusterListResponse func() interface{} exampleOperationListResponse func() interface{} xmsEnum []string commonTypesVersion string systemData bool } var apis = map[string]*api { apiv20200430Path: { exampleOpenShiftClusterPutParameter: v20200430.ExampleOpenShiftClusterPutParameter, // ... }, // ... }

Can we do this in go?

cannot use v20200430.ExampleOpenShiftClusterPutParameter (value of type func() *v20200430.OpenShiftCluster) as func() interface{} value in struct literal

was sitting on this for 2 hours trying to understand what you had in mind here....

foo func() interface{} != func foo() *Custom{

https://play.golang.org/p/jUb0aqTLP5s

Before I start refactoring for 4th time I would like to understand if Im missing something here for this to work (?)

This caught my curiosity and found this interesting answer.
https://stackoverflow.com/a/54751503
I still need to see if the same applies to return values

@mjudeikis I'm inclined to do the horrible thing and make all the Example*() functions return interface{}. It's rubbish but it's a lot less boilerplate.

pkg/swagger/swagger.go

pkg/swagger/generator.go

jim-minter · 2021-02-10T20:13:05Z

pkg/swagger/swagger.go

@@ -73,16 +93,18 @@ func Run(api, outputDir string) error {

 	xmsEnumList := map[string]struct{}{}

-	if api != "github.com/Azure/ARO-RP/pkg/api/v20200430" {
+	names := []string{"OpenShiftClusterList", "OpenShiftClusterCredentials"}


I think a lot of the code around here could be more neatly parameterised and defined in g

...ft.RedHatOpenShift/preview/2021-01-31-preview/examples/OpenShiftClusters_CreateOrUpdate.json

jim-minter · 2021-02-10T20:48:13Z

pkg/swagger/swagger.go

@@ -126,6 +148,19 @@ func Run(api, outputDir string) error {
 		}

 		s.Definitions[azureResource].Properties = properties
+
+		if g.Features().systemData {


this doesn't look right. I think it is masking the fact that we actually need to add this to the model and deal with it. See https://github.com/Azure/azure-resource-manager-rpc/blob/master/v1.0/common-api-contracts.md#system-metadata-for-all-azure-resources

Added separate PR for this. Sill in draft, but working on it #1412

jim-minter · 2021-02-10T20:48:13Z

pkg/swagger/types.go

@@ -164,6 +164,7 @@ type Response struct {
 	Schema      *Schema `json:"schema,omitempty"`
 	Headers     Headers `json:"headers,omitempty"`
 	Examples    Example `json:"examples,omitempty"`
+	SystemData  *Schema `json:"systemData,omitempty"`


what is this for?

ARM CI/sign-off required this for all new APIs/API Versions. I think we will need to make change not to log this in any away in our code but overall we need it now

Not sure that I understand - how is it related to the response object?

jim-minter · 2021-03-02T19:43:11Z

bump - there's a longstanding unactioned review here

mjudeikis · 2021-03-24T13:41:59Z

pkg/swagger/swagger.go

@@ -126,6 +142,19 @@ func Run(api, outputDir string) error {
 		}

 		s.Definitions[azureResource].Properties = properties
+
+		if g.systemData {
+			s.Definitions[azureResource].Properties = append(s.Definitions[azureResource].Properties,


wondering if this is right?

mjudeikis · 2021-04-21T12:02:58Z

/azp run e2e

azure-pipelines · 2021-04-21T12:03:08Z

Azure Pipelines successfully started running 1 pipeline(s).

github-actions · 2021-04-21T12:13:40Z

Please rebase pull request.

mjudeikis · 2021-04-22T13:36:44Z

/azp run e2e

azure-pipelines · 2021-04-22T13:36:56Z

Azure Pipelines successfully started running 1 pipeline(s).

mjudeikis · 2021-04-22T18:25:00Z

/azp run e2e

azure-pipelines · 2021-04-22T18:25:11Z

Azure Pipelines successfully started running 1 pipeline(s).

mjudeikis · 2021-04-23T09:40:19Z

/azp run e2e

azure-pipelines · 2021-04-23T09:40:28Z

Azure Pipelines successfully started running 1 pipeline(s).

m1kola

I like how you introduced support for generating multiple api versions and their examples, but I'm not sure that changes related to systemData and admin kube config are correct. For instance this PR introduces a new endpoint to list the admin kube config. Are we ready to make this decision?

I'm up for merging the generator part, but not sure that I'm comfortable with placeholders for new features and related swagger generation changes. If you want - we can split this into multiple pull requests so we can merge the generator part faster.

m1kola · 2021-04-23T13:44:50Z

pkg/swagger/types.go

@@ -164,6 +164,7 @@ type Response struct {
 	Schema      *Schema `json:"schema,omitempty"`
 	Headers     Headers `json:"headers,omitempty"`
 	Examples    Example `json:"examples,omitempty"`
+	SystemData  *Schema `json:"systemData,omitempty"`


Not sure that I understand - how is it related to the response object?

m1kola · 2021-04-23T13:49:26Z

pkg/swagger/swagger.go

 		},
 	}
+	if g.kubeconfig {
+		s.Paths["/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.RedHatOpenShift/openShiftClusters/{resourceName}/listAdminCredentials"] = &PathItem{


This implies that we will have another endpoint (and another az cli command) to get admin kubeconfig. Why not extend existing command/endpoint for the new verson?

Yes, this will be different endpoint and API. Few reasons for this:

Prevent api breakage for old API

API purpose is different as it will download file, and not expose

Easier to implement.

It will return privileged kubeconfig so in the future dedicated rbac could be added

m1kola · 2021-04-23T14:20:10Z

pkg/swagger/swagger.go

@@ -24,7 +29,7 @@ func Run(api, outputDir string) error {
 		Schemes:     []string{"https"},
 		Consumes:    []string{"application/json"},
 		Produces:    []string{"application/json"},
-		Paths:       populateTopLevelPaths("Microsoft.RedHatOpenShift", "openShiftCluster", "OpenShift cluster"),
+		Paths:       g.populateTopLevelPaths("Microsoft.RedHatOpenShift", "openShiftCluster", "OpenShift cluster"),


Can we put resourceProviderNamespace as a parameter into generator struct so we don't have to pass it here?

Not on this PR, or one if follow-up. Too much change.

m1kola · 2021-04-23T14:27:44Z

pkg/swagger/swagger.go

+				Responses:   g.populateResponses("OpenShiftClusterAdminKubeconfig", false, http.StatusOK),
+			},
+		}
+	}

 	s.Paths["/providers/Microsoft.RedHatOpenShift/operations"] = &PathItem{


I think we need to move listCredentials, listAdminCredentials (if we actually need it, see the question above) and operations paths somewhere into generator. Similarly to populateTopLevelPaths.

And probably all logic from Run into generator so the whole Run function will be something like:

g, err := New(api, params1, param2) if err != nil { return err } s := g.Swagger() b, err := json.MarshalIndent(s, "", " ") if err != nil { return err } b = append(b, '\n') return ioutil.WriteFile(outputDir+"/redhatopenshift.json", b, 0666)

But I think it should be a separate PR: this one is already too big.

mjudeikis · 2021-04-23T15:29:20Z

I like how you introduced support for generating multiple api versions and their examples, but I'm not sure that changes related to systemData and admin kube config are correct. For instance this PR introduces a new endpoint to list the admin kube config. Are we ready to make this decision?

I'm up for merging the generator part, but not sure that I'm comfortable with placeholders for new features and related swagger generation changes. If you want - we can split this into multiple pull requests so we can merge the generator part faster.

I think I gonna split the PRs as you suggest. Initially placeholder where there to kick start arm review process on API so could do implementation in parallel. We still need to do this, but maybe separate PR.

mjudeikis · 2021-04-26T09:37:56Z

will re-open in smallers prs

jim-minter reviewed Dec 2, 2020

View reviewed changes

pkg/api/v20210131preview/openshiftclustercredentials.go Outdated Show resolved Hide resolved

pkg/api/v20210131preview/openshiftclustercredentials.go Outdated Show resolved Hide resolved

pkg/api/v20210131preview/openshiftclustercredentials.go Outdated Show resolved Hide resolved

github-actions bot added the needs-rebase branch needs a rebase label Dec 4, 2020

mjudeikis force-pushed the api.skeleton branch 2 times, most recently from e05269f to b3b0a69 Compare December 14, 2020 12:08

github-actions bot removed the needs-rebase branch needs a rebase label Dec 14, 2020

mjudeikis marked this pull request as ready for review December 14, 2020 12:10

mjudeikis requested a review from olga-mir as a code owner December 14, 2020 12:10

mjudeikis force-pushed the api.skeleton branch 3 times, most recently from 61479b8 to e7bf5d5 Compare December 14, 2020 12:49

jim-minter suggested changes Dec 15, 2020

View reviewed changes

mjudeikis force-pushed the api.skeleton branch 3 times, most recently from 020f3fd to c0a5aea Compare December 16, 2020 14:33

mjudeikis added the next-up label Dec 16, 2020

mjudeikis force-pushed the api.skeleton branch from 52c59bc to d70bf42 Compare January 4, 2021 14:51

mjudeikis added this to the Sprint 196 milestone Jan 27, 2021

jim-minter reviewed Jan 27, 2021

View reviewed changes

pkg/api/v20210131preview/openshiftclusteradminkubeconfig_example.go Outdated Show resolved Hide resolved

pkg/swagger/examples.go Outdated Show resolved Hide resolved

pkg/swagger/swagger.go Outdated Show resolved Hide resolved

mjudeikis force-pushed the api.skeleton branch 3 times, most recently from eeec873 to e4ae89f Compare February 9, 2021 12:06

jim-minter suggested changes Feb 10, 2021

View reviewed changes

jim-minter removed the next-up label Feb 18, 2021

jim-minter removed this from the Sprint 196 milestone Feb 18, 2021

mjudeikis added next-up and removed next-up labels Feb 24, 2021

mjudeikis force-pushed the api.skeleton branch from e4ae89f to 0cb1ae3 Compare March 24, 2021 10:40

mjudeikis requested a review from jewzaam as a code owner March 24, 2021 10:40

mjudeikis force-pushed the api.skeleton branch from 0cb1ae3 to 6f674e3 Compare March 24, 2021 13:29

mjudeikis mentioned this pull request Mar 24, 2021

Add systemData handling #1412

Merged

mjudeikis commented Mar 24, 2021

View reviewed changes

mjudeikis force-pushed the api.skeleton branch from 6f674e3 to 0f64608 Compare March 24, 2021 13:48

mjudeikis added the ready-for-review label Mar 24, 2021

mjudeikis force-pushed the api.skeleton branch from 0f64608 to 726ad2b Compare April 21, 2021 12:13

github-actions bot added the needs-rebase branch needs a rebase label Apr 21, 2021

mjudeikis force-pushed the api.skeleton branch from 726ad2b to 3375439 Compare April 21, 2021 12:15

github-actions bot removed the needs-rebase branch needs a rebase label Apr 21, 2021

mjudeikis added 3 commits April 21, 2021 14:13

systemData and generator for multiapi

c0159dc

Add new api fields

2183661

generate

336aad4

mjudeikis force-pushed the api.skeleton branch from 3375439 to 336aad4 Compare April 21, 2021 13:22

m1kola reviewed Apr 23, 2021

View reviewed changes

mjudeikis mentioned this pull request Apr 26, 2021

Multi API generator refactor #1458

Merged

mjudeikis closed this Apr 26, 2021

API Skeleton 2021-01-31 - swagger #1200

API Skeleton 2021-01-31 - swagger #1200

Conversation

mjudeikis commented Nov 27, 2020 • edited Loading

jim-minter left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2020

jim-minter left a comment

Choose a reason for hiding this comment

mjudeikis commented Dec 18, 2020

mjudeikis commented Jan 4, 2021

mjudeikis commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanderpotte Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter commented Mar 2, 2021

Choose a reason for hiding this comment

mjudeikis commented Apr 21, 2021

azure-pipelines bot commented Apr 21, 2021

github-actions bot commented Apr 21, 2021

mjudeikis commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

mjudeikis commented Apr 22, 2021

azure-pipelines bot commented Apr 22, 2021

mjudeikis commented Apr 23, 2021

azure-pipelines bot commented Apr 23, 2021

m1kola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjudeikis commented Apr 23, 2021

mjudeikis commented Apr 26, 2021

mjudeikis commented Nov 27, 2020 •

edited

Loading

gvanderpotte Mar 3, 2021 •

edited

Loading