2021-01-31 kickoff #1191

mjudeikis · 2020-11-25T11:04:30Z

Which issue this PR addresses:

Enables us to start developing new API 2020-01-31-preview and test it in dev

What this PR does / why we need it:

~~This will make an extension to call preview API exclusively.~~
~~If you are using an extension to interact with production RP - this will stop working. Please offload the extension for these activities.~~
Pulled back ^ as we might want to discuss more. But I have a feeling this will be "Told you so". Sometimes we have to break things for sake of progress...
All E2E in dev will be running on preview API.
All E2E in PROD and INT will be running on stable API

Next will be swagger and cli generation WITH new fields and submission initial swagger for review: https://github.com/Azure/ARO-RP/compare/master...mjudeikis:api.skeleton?expand=1

Test plan for issue:

In development

Is there any documentation that needs to be updated for this PR?

This is just the beginning. Later - yet.

mjudeikis · 2020-11-25T13:21:52Z

/azp run e2e

azure-pipelines · 2020-11-25T13:22:04Z

Azure Pipelines successfully started running 1 pipeline(s).

jim-minter · 2020-11-25T15:10:43Z

pkg/util/azureclient/mgmt/redhatopenshiftpreview/openshiftclusters.go

@@ -0,0 +1,53 @@
+package redhatopenshiftpreview


shall we just admit defeat and go to pkg/util/azureclient/mgmt/redhatopenshift/2021-01-31-preview/redhatopenshift ?

jim-minter · 2020-11-25T15:11:18Z

pkg/util/cluster/cluster.go

+	groups                   features.ResourceGroupsClient
+	applications             graphrbac.ApplicationsClient
+	serviceprincipals        graphrbac.ServicePrincipalClient
+	openshiftclusters        redhatopenshift.OpenShiftClustersClient


call this something awful like openshiftclusters20200430 or something

jim-minter · 2020-11-25T15:13:34Z

pkg/util/cluster/cluster.go

@@ -368,6 +388,54 @@ func (c *Cluster) createCluster(ctx context.Context, clusterName, clientID, clie
 	return c.openshiftclusters.CreateOrUpdateAndWait(ctx, c.ResourceGroup(), clusterName, oc)
 }

+func (c *Cluster) createClusterPreview(ctx context.Context, clusterName, clientID, clientSecret string) error {


investigate to see if we can define the cluster in the internal representation and use api converters to generate the correct api manifest for the version of client that's being used

jim-minter · 2020-11-25T15:15:30Z

pkg/util/cluster/cluster.go

+			err = c.openshiftclusterspreview.DeleteAndWait(ctx, c.ResourceGroup(), clusterName)
+			if err != nil {
+				errs = append(errs, err)
+			}
+		} else {
+			err := c.openshiftclusters.DeleteAndWait(ctx, c.ResourceGroup(), clusterName)
+			if err != nil {
+				errs = append(errs, err)
+			}


I'm not sure this is worth it

jim-minter · 2020-11-25T15:17:14Z

python/az/aro/azext_aro/_client_factory.py

@@ -4,7 +4,7 @@
 import urllib3

 from azext_aro.custom import rp_mode_development
-from azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2020_04_30 import AzureRedHatOpenShiftClient
+from azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2021_01_31_preview import AzureRedHatOpenShiftClient


Let's think about and discuss the az side of this a bit more.

github-actions · 2020-11-26T02:12:54Z

Please rebase pull request.

mjudeikis · 2020-11-26T11:31:20Z

pkg/util/cluster/cluster.go

+	"github.com/Azure/ARO-RP/pkg/api"
+	v20200430 "github.com/Azure/ARO-RP/pkg/api/v20200430"
+	v2021131preview "github.com/Azure/ARO-RP/pkg/api/v20210131preview"
+	mgmtopenshiftclustersv20200430 "github.com/Azure/ARO-RP/pkg/client/services/redhatopenshift/mgmt/2020-04-30/redhatopenshift"
+	mgmtopenshiftclustersv20210131preview "github.com/Azure/ARO-RP/pkg/client/services/redhatopenshift/mgmt/2021-01-31-preview/redhatopenshift"


I dont know what is better. Keeping this or having createCluster 2 methods...

mjudeikis · 2020-11-26T11:31:48Z

pkg/util/cluster/cluster.go

+		ext := api.APIs[v20200430.APIVersion].OpenShiftClusterConverter().ToExternal(&oc)
+		data, err := json.Marshal(ext)
+		if err != nil {
+			return err
+		}
+
+		ocExt := mgmtopenshiftclustersv20200430.OpenShiftCluster{}
+		err = json.Unmarshal(data, &ocExt)
+		if err != nil {
+			return err
+		}
+
+		return c.openshiftclustersv20200430.CreateOrUpdateAndWait(ctx, c.env.ResourceGroup(), clusterName, ocExt)


This is where create magic happens.

mjudeikis · 2020-11-26T11:32:12Z

python/az/aro/azext_aro/custom.py

@@ -4,7 +4,7 @@
 import random
 import os

-import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2020_04_30.models as v2020_04_30
+import azext_aro.vendored_sdks.azure.mgmt.redhatopenshift.v2020_04_30.models as openshiftcluster


Will make future renames easier when we agree what we want to do with this

mjudeikis · 2020-11-26T11:34:32Z

test/e2e/adminapi_resources.go

@@ -26,7 +26,7 @@ var _ = Describe("[Admin API] List Azure resources action", func() {
 		resourceID := resourceIDFromEnv()

 		By("getting the resource group where cluster resources live in")
-		oc, err := clients.OpenshiftClusters.Get(ctx, _env.ResourceGroup(), clusterName)
+		oc, err := clients.OpenshiftClustersv20200430.Get(ctx, _env.ResourceGroup(), clusterName)


For now E2E are using old API. So this basically scatters our e2e. Cluster will be created with new API. So any new future tests should be not API binded. In example.
If we create FIPs cluster, we should write tests which currently run in dev only and uses Kubernetes API to check it. Not ARO...

💩 a bit. Not sure how we can do this without copy-paste test cases.

I don't agree with this or see the problem. Let's discuss tomorrow.

mjudeikis · 2020-11-26T12:05:36Z

/azp run e2e

azure-pipelines · 2020-11-26T12:05:48Z

Azure Pipelines successfully started running 1 pipeline(s).

jim-minter

Minor fixes requested. I'm OK with this.

...oft.RedHatOpenShift/stable/2021-01-31-preview/examples/OpenShiftClusters_CreateOrUpdate.json

swagger/redhatopenshift/resource-manager/readme.md

...ft/resource-manager/Microsoft.RedHatOpenShift/stable/2021-01-31-preview/redhatopenshift.json

swagger/redhatopenshift/resource-manager/readme.go.md

jim-minter · 2020-12-01T23:55:07Z

test/e2e/adminapi_resources.go

@@ -26,7 +26,7 @@ var _ = Describe("[Admin API] List Azure resources action", func() {
 		resourceID := resourceIDFromEnv()

 		By("getting the resource group where cluster resources live in")
-		oc, err := clients.OpenshiftClusters.Get(ctx, _env.ResourceGroup(), clusterName)
+		oc, err := clients.OpenshiftClustersv20200430.Get(ctx, _env.ResourceGroup(), clusterName)


I don't agree with this or see the problem. Let's discuss tomorrow.

jim-minter

I believe this is good, if @mjudeikis does.

mjudeikis force-pushed the api.kickoff branch from dc30a27 to 763c506 Compare November 25, 2020 11:38

jim-minter reviewed Nov 25, 2020

View reviewed changes

github-actions bot added the needs-rebase branch needs a rebase label Nov 26, 2020

mjudeikis force-pushed the api.kickoff branch from 3e15dc0 to 022fe2e Compare November 26, 2020 11:29

github-actions bot removed the needs-rebase branch needs a rebase label Nov 26, 2020

mjudeikis commented Nov 26, 2020

View reviewed changes

mjudeikis force-pushed the api.kickoff branch from 022fe2e to 069c8dc Compare November 26, 2020 11:37

mjudeikis marked this pull request as ready for review November 26, 2020 11:37

mjudeikis requested review from ehashman and olga-mir as code owners November 26, 2020 11:37

mjudeikis added the next-up label Nov 26, 2020

mjudeikis mentioned this pull request Nov 27, 2020

API Skeleton 2021-01-31 - swagger #1200

Closed

jim-minter suggested changes Dec 1, 2020

View reviewed changes

mjudeikis force-pushed the api.kickoff branch 2 times, most recently from cdc2237 to bf946be Compare December 2, 2020 13:27

2020-10-31-preview -> 2021-01-31-preview

c19f411

jim-minter force-pushed the api.kickoff branch from bf946be to 65e048b Compare December 4, 2020 14:24

jim-minter previously approved these changes Dec 4, 2020

View reviewed changes

fixups

184f35e

jim-minter dismissed their stale review via 184f35e December 4, 2020 15:39

jim-minter force-pushed the api.kickoff branch from 65e048b to 184f35e Compare December 4, 2020 15:39

jim-minter merged commit 4780834 into Azure:master Dec 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2021-01-31 kickoff #1191

2021-01-31 kickoff #1191

mjudeikis commented Nov 25, 2020 •

edited

Loading

mjudeikis commented Nov 25, 2020

azure-pipelines bot commented Nov 25, 2020

jim-minter Nov 25, 2020

jim-minter Nov 25, 2020

jim-minter Nov 25, 2020

jim-minter Nov 25, 2020

jim-minter Nov 25, 2020

github-actions bot commented Nov 26, 2020

mjudeikis Nov 26, 2020

mjudeikis Nov 26, 2020

mjudeikis Nov 26, 2020

mjudeikis Nov 26, 2020

jim-minter Dec 1, 2020

mjudeikis commented Nov 26, 2020

azure-pipelines bot commented Nov 26, 2020

jim-minter left a comment

jim-minter Dec 1, 2020

jim-minter left a comment

2021-01-31 kickoff #1191

2021-01-31 kickoff #1191

Conversation

mjudeikis commented Nov 25, 2020 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

mjudeikis commented Nov 25, 2020

azure-pipelines bot commented Nov 25, 2020

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

github-actions bot commented Nov 26, 2020

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 Nov 26, 2020

azure-pipelines bot commented Nov 26, 2020

jim-minter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jim-minter left a comment

Choose a reason for hiding this comment

mjudeikis commented Nov 25, 2020 •

edited

Loading