Container Service - Add OpenShift support #3712

julienstroheker · 2018-08-23T18:44:21Z

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-08-23T18:45:01Z

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#3378

azuresdkci · 2018-08-23T18:45:10Z

Can one of the admins verify this patch?

AutorestCI · 2018-08-23T18:45:32Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-08-23T18:46:37Z

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#3013

AutorestCI · 2018-08-23T18:52:37Z

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3411

AutorestCI · 2018-08-23T18:55:52Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

annatisch · 2018-08-23T23:49:18Z

Thanks @julienstroheker - please ping me when you've finished updating the spec (i.e. no longer WIP) so I can tag for ARM review. :)

lmazuel · 2018-09-07T21:52:41Z

@AutorestCI rebuild azure-sdk-for-python

AutorestCI · 2018-09-20T22:35:06Z

Automation for azure-sdk-for-js

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-js#181

julienstroheker · 2018-09-24T17:23:53Z

@amanohar @mboersma

annatisch

Hi @julienstroheker - I just did a preliminary look over while we're waiting on ARM signoff.

annatisch · 2018-09-25T15:45:25Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+      },
+      "description": "Tags object for patch operations."
+    },
+    "OpenShiftManagedClusterListResult": {


This definition is never used? Perhaps missing a "list" operation?

Good catch. Not for now but for sure later. I'll remove it for now.

annatisch · 2018-09-25T15:46:40Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+      ],
+      "description": "OpenShift Managed cluster."
+    },
+    "OpenShiftManagedClusterServiceAADIdentityProvider": {


This definition is never used?

This is used in OpenShiftManagedClusterIdentityProviders in the provider field as object we are planning to support more auth in the future

But this generated model will not be used in response deserialization?

It will be used by the sdk to create an authentication model? Makes sense ?

Hi @julienstroheker ,
Yes and no - technically this will work, however the standard approach here is to use polymorphic models. I see for example, that OpenShiftManagedClusterServiceAADIdentityProvider has a "kind" property. We generally consider this to be a discriminator (as I suspect this should be a constant value?). If all your intended "IdentityProvider" models will likely have a "kind" property - then I would recommend that you restructure this to use an "OpenShiftManagedClusterServiceIdentityProvider" base class.

Here is an example:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/postgresql/resource-manager/Microsoft.DBforPostgreSQL/stable/2017-12-01/postgresql.json#L1355

You can see the base class "ServerPropertiesForCreate" defines the descriminator "createMode" (in your case this would be "kind") which is then set for each of the children using the "x-ms-discriminator-value" extension. In this example the base class has additional common properties - but that's not required depending on your case.

Here is an additional example:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/graphrbac/data-plane/stable/1.6/graphrbac.json#L2327

In this case, like yours, the base class only has one available child class at present - which leaves them the option to add more later.

Thanks @julienstroheker - But making the model polymorphic doesn't change the interface?

What is the expected value of "kind" for the AAD provider? It seems that at a minimum this should be a constant (i.e. an enum with a single option) so that a user doesn't have to set this value.

This should works indeed ! Let me run some tests with it ! Thanks @annatisch

@annatisch I just pushed the modification, can you check and tell me if it is ok?

Thanks @julienstroheker - not quite - You can remove the x-ms-enum extension and it also needs to be marked as required.
Take a look at the docs here:
https://github.com/Azure/autorest/blob/bc0dcde0a05606a1b102c0e08c1798fa4da4dcc8/docs/extensions/readme.md#single-value-enum-as-a-constant

What about now ? :)

annatisch · 2018-09-25T15:51:18Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+          "description": "Type of authentication profile to use."
+        }
+      },
+      "description": "OpenShiftManagedClusterAuthProfile defines all possible authentication profiles for the OpenShift cluster."


Try to avoid using the name of the model or property in the description (i.e. OpenShiftManagedClusterAuthProfile) as this may not be the way it appears in a generated SDK depending on the naming conventions of the language.

In this instance I think you could simply removed it:
"description": "Defines all possible authentication profiles for the OpenShift cluster"

annatisch · 2018-09-25T15:55:32Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+          "description": "Configuration of the provider."
+        }
+      },
+      "description": "OpenShiftManagedClusterIdentityProvider is heavily cut down equivalent to IdentityProvider in the upstream."


This description would be a bit confusing to a developer using the SDK.
I think it would be better to give a simple description of its role, e.g. "Basic/Essential/etc details of an Identity Provider"

annatisch · 2018-09-25T15:57:52Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+        "count",
+        "vmSize"
+      ],
+      "description": "OpenShiftManagedClusterAgentPoolProfile represents configuration of OpenShift cluster VMs."


Try to avoid using the name of the model or property in the description (i.e. OpenShiftManagedClusterAgentPoolProfile) as this may not be the way it appears in a generated SDK depending on the naming conventions of the language.

In this instance I think you could simply removed it:
"description": "Represents configuration of OpenShift cluster VMs."

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

annatisch · 2018-09-25T16:14:43Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+    }
+  },
+  "definitions": {
+    "OperationListResult": {


This spec does not include any "Operations" requests - so I don't think this and the next two models need to be here.

annatisch · 2018-09-25T17:07:51Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+          "maximum": 5,
+          "minimum": 1,
+          "description": "Number of agents (VMs) to host docker containers. Allowed values must be in the range of 1 to 5 (inclusive). The default value is 1. ",
+          "default": 1


I quick comment on the use of "defaults". I see you have a couple of them in the spec, so I wont leave a comment for every one.
It's important to note that how "defaults" are handled can vary from language to language.
Some languages will send the default values in the request body (regardless of whether it's set by the user) and other languages will leave these fields in the request as empty, and use the default purely to document the service behavior.

So please make sure that your service API will handle both scenarios the same way - i.e. it will treat a request with an empty field and a request with a default value exactly the same.

annatisch · 2018-09-28T21:14:00Z

ping @ravbhatnagar

julienstroheker · 2018-10-02T20:50:49Z

@annatisch @ravbhatnagar any news on this ? Thanks

ravbhatnagar

Few notes added. please take a look.

ravbhatnagar · 2018-10-02T20:54:18Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+            "description": "Parameters supplied to the Create or Update an OpenShift Managed Cluster operation."
+          }
+        ],
+        "responses": {


please model the default response to capture all the error states. Sample can be found in batch RP swagger.

Schema has not been added for the default response. Please check Batch RP swagger for sample.

ravbhatnagar · 2018-10-02T20:55:11Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+            "description": "The name of the openshift managed cluster resource."
+          }
+        ],
+        "responses": {


model the 200 response. 204 will indicate that the resource to be deleted was not found. and 200 will indicate that the resource was found and deleted successfully.

Hi @ravbhatnagar we don't have a 200 response here, only 202 or 204. is it ok ?

so as per ARM Resource provider contract, 204 on a delete must be returned when the requested resourceId was not found. 200 is returned in case where resource was successfully deleted.

ravbhatnagar · 2018-10-02T20:56:44Z

...-manager/Microsoft.ContainerService/preview/2018-09-30-preview/openShiftManagedClusters.json

+      }
+    }
+  },
+  "paths": {


GET APIs to list all openShiftManagedClusters under a resource group and under a subscription missing. Please add.

It is not implemented yet in our RP.

ok, please note that having this is a requirement as clients like portal will have issues if these APIs were not supported.

ravbhatnagar · 2018-10-08T18:51:05Z

Only one outstanding item- adding the schema for the default response. Conditionally Signing off from ARM side.

# Conflicts: # specification/containerservices/resource-manager/readme.md

julienstroheker · 2018-10-10T15:46:36Z

@ravbhatnagar @annatisch I added the schema for the default response. Please let me know if it is ok ? Thanks

annatisch · 2018-10-11T19:13:59Z

Thanks for the updates @julienstroheker - I'm just about to do a final review.
I see that you've bumped the Python version as a patch release - just checking, are you aware that in some languages there are potentially breaking changes in this PR?

add openshift support

fdda83f

AutorestCI mentioned this pull request Aug 23, 2018

[AutoPR containerservices/resource-manager] [WIP] - Container Service - Add OpenShift support Azure/azure-sdk-for-python#3190

Closed

azuresdkci assigned annatisch Aug 23, 2018

azuresdkci requested a review from annatisch August 23, 2018 18:45

update python sdk version

30e98cc

julienstroheker added 3 commits August 23, 2018 15:14

including others definitions

857efa1

bump default version

900ca4e

remove operation duplicate

4455057

AutorestCI mentioned this pull request Aug 23, 2018

[AutoPR containerservices/resource-manager] [WIP] - Container Service - Add OpenShift support Azure/azure-sdk-for-node#3411

Closed

julienstroheker added 4 commits September 4, 2018 10:04

Update the model

c7bdb06

update OpenShift API

60b6d7a

typo and update model

67b876b

add ClusterAuthProfile definition

320a8eb

AutorestCI mentioned this pull request Sep 7, 2018

[AutoPR containerservices/resource-manager] [WIP] - Container Service - Add OpenShift support Azure/azure-sdk-for-python#3301

Closed

julienstroheker added 2 commits September 7, 2018 22:10

change params name

7b99cf0

Clean and update osa deifinitions

f730c88

julienstroheker mentioned this pull request Sep 20, 2018

[WIP] - Add OpenShift support Azure/azure-cli#7385

Closed

2 tasks

annatisch added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Sep 21, 2018

Add PATCH route and update examples

9bbc8d7

julienstroheker changed the title ~~[WIP] - Container Service - Add OpenShift support~~ Container Service - Add OpenShift support Sep 24, 2018

annatisch reviewed Sep 25, 2018

View reviewed changes

julienstroheker added 2 commits September 25, 2018 15:57

Add golang package

5fec66c

Apply review

32ce1d9

AutorestCI mentioned this pull request Sep 25, 2018

[AutoPR containerservices/resource-manager] Container Service - Add OpenShift support Azure/azure-sdk-for-go#2819

Merged

Update Vnet Integration + Examples

1dae12f

ravbhatnagar reviewed Oct 2, 2018

View reviewed changes

ravbhatnagar added the ARMReviewInProgress label Oct 2, 2018

julienstroheker added 2 commits October 2, 2018 17:44

Caps / no Caps

8043740

Add default response

2ea238b

ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Oct 8, 2018

julienstroheker added 4 commits October 10, 2018 11:27

adding the schema for the default response

0d9c415

Merge branch 'master' into containerservice-openshift

f940276

# Conflicts: # specification/containerservices/resource-manager/readme.md

Add Go Package

06f0477

Bump Python version

2f9a701

julienstroheker added 2 commits October 11, 2018 10:31

Enum kind of OpenShiftManagedClusterServiceAADIdentityProvider

a3e455b

Add Kind as required

1654a19

Adding OpenShiftManagedClusterBaseIdentityProvider and rename properties

4e40c60

AutorestCI mentioned this pull request Oct 12, 2018

[AutoPR containerservices/resource-manager] Container Service - Add OpenShift support Azure/azure-sdk-for-js#138

Merged

julienstroheker added 3 commits October 12, 2018 11:52

Sync Python version

f85fed8

Bump Python version

9a7d174

Merge branch 'master' into containerservice-openshift

fe1e5ab

annatisch approved these changes Oct 12, 2018

View reviewed changes

annatisch merged commit d37ec27 into Azure:master Oct 15, 2018

Container Service - Add OpenShift support #3712

Container Service - Add OpenShift support #3712

Conversation

julienstroheker commented Aug 23, 2018

PR information

Quality of Swagger

AutorestCI commented Aug 23, 2018 • edited Loading

Automation for azure-sdk-for-python

azuresdkci commented Aug 23, 2018

AutorestCI commented Aug 23, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Aug 23, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Aug 23, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Aug 23, 2018 • edited Loading

Automation for azure-sdk-for-java

annatisch commented Aug 23, 2018

lmazuel commented Sep 7, 2018

AutorestCI commented Sep 20, 2018 • edited Loading

Automation for azure-sdk-for-js

julienstroheker commented Sep 24, 2018

annatisch 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

julienstroheker Sep 25, 2018 • 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

julienstroheker Oct 11, 2018 • 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

annatisch commented Sep 28, 2018

julienstroheker commented Oct 2, 2018

ravbhatnagar 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravbhatnagar commented Oct 8, 2018

julienstroheker commented Oct 10, 2018

annatisch commented Oct 11, 2018

AutorestCI commented Aug 23, 2018 •

edited

Loading

AutorestCI commented Aug 23, 2018 •

edited

Loading

AutorestCI commented Aug 23, 2018 •

edited

Loading

AutorestCI commented Aug 23, 2018 •

edited

Loading

AutorestCI commented Aug 23, 2018 •

edited

Loading

AutorestCI commented Sep 20, 2018 •

edited

Loading

julienstroheker Sep 25, 2018 •

edited

Loading

julienstroheker Oct 11, 2018 •

edited

Loading