[WIP] - Add OpenShift support by julienstroheker · Pull Request #7385 · Azure/azure-cli

julienstroheker · 2018-09-20T22:55:12Z

Please do not merge WIP

Related to :

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

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

troydai

Still reviewing it. One comment for now.

troydai · 2018-09-20T23:36:33Z

src/azure-cli-testsdk/azure/cli/testsdk/__init__.py


 __all__ = ['ScenarioTest', 'LiveScenarioTest', 'ResourceGroupPreparer', 'StorageAccountPreparer',
-           'RoleBasedServicePrincipalPreparer', 'CliTestError', 'JMESPathCheck', 'JMESPathCheckExists', 'NoneCheck',
+           'RoleBasedServicePrincipalPreparer', 'ManagedApplicationPreparer', 'CliTestError', 'JMESPathCheck', 'JMESPathCheckExists', 'NoneCheck',


Break the line at 120.

troydai · 2018-09-20T23:37:35Z

src/azure-cli-testsdk/azure/cli/testsdk/preparers.py

+            self.test_class_instance.kwargs[self.key] = name
+            # The slice is the easiest way for know to return the Teanant from the same command
+            return {self.parameter_name: self.result['appId'], self.parameter_secret: name,
+                    self.parameter_tenant: self.result['odata.metadata'][26:62]}


This not very stable. Isn't there any alternative?

Indeed, it is not very clean, the response from the command is not returning this field tenantID this is why I did this, to test.

I can add another cli command such as az account list to get this info though ?

Sounds like a more stable solution than this. Please do that and also add comments regarding why it invokes that command to fetch the tenant id.

Add _ensure_osa_aad

# Conflicts: # src/command_modules/azure-cli-acs/HISTORY.rst # src/command_modules/azure-cli-acs/setup.py

src/azure-cli-testsdk/azure/cli/testsdk/preparers.py

troydai · 2018-10-01T16:20:21Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+                    aad_tenant_id=None,
+                    identifier=None,
+                    name=None):
+    # TODO: This really needs to be unit tested.


Please add the unit tests.

Sorry I forgot to remove that comment, Can you tell me if my tests in test_osa_commands.py are ok ?

i think so.

troydai · 2018-10-01T16:21:55Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

                       ' The available locations are "{}"'.format(','.join(aci_locations)))
+
+
+def openshift_create(cmd, client, resource_group_name, name,  # pylint: disable=too-many-locals


Break this method into smaller methods to avoid too-many-locals warning.

I am not sure to understand how I can do this. Those parameters are from the user. I am doing the same logic as the aks_create fct around line 1415. Can you give me some hints here ?

"too many locals" means that you defined too many local variables in this method, which indicates that the body of the function is too large.

troydai · 2018-10-01T16:24:08Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

+
+    # We don't creating the AADIdentity for the user right now but maybe later so keeping this
+    # Keeping this Due to SPN replication latency, we do a few retries here
+    max_retry = 30


This will make the retry last up to 90 seconds. I recommend you print warning to the stderr through logging so as to avoid the impression of command hanging.

I removed that part since I added the auto creation of the AADIdentity : _ensure_osa_aad

troydai · 2018-10-01T16:24:33Z

...ommand_modules/azure-cli-acs/azure/cli/command_modules/acs/tests/latest/test_osa_commands.py

+    ResourceGroupPreparer, ManagedApplicationPreparer, ScenarioTest, live_only)
+from azure_devtools.scenario_tests import AllowLargeResponse
+from azure.cli.testsdk.checkers import (
+    StringContainCheck, StringContainCheckIgnoreCase)


Keep in the line.

Add _remove_nulls_osa Update NetworkProfile

Rename Models Rename node-count to compute-count fqdn mandatory

troydai · 2018-10-25T07:15:01Z

Please remove me from the reviewers list.

42B · 2018-10-28T16:41:05Z

src/azure-cli-testsdk/azure/cli/testsdk/preparers.py

+        self.result = {}
+        self.app_name = app_name
+        self.dev_setting_app_name = os.environ.get(dev_setting_app_name, None)
+        self.dev_setting_app_secret = os.environ.get(dev_setting_app_secret, None)


Isn't the default value for os.environ.get already None?

Rename node-vm-size to compute-vm-size

tjprescott · 2018-12-13T16:45:51Z

Closing due to inactivity. Feel free to reopen when the PR is ready.

julienstroheker · 2018-12-13T19:07:19Z

Hi @tjprescott waiting after : Azure/azure-sdk-for-python#3378 to be merged

tjprescott · 2018-12-13T19:53:00Z

Hi @julienstroheker no worries. Feel free to reopen when the Python SDK is merged.

julienstroheker added 3 commits September 20, 2018 18:45

Add OSA create show delete scale

c87021b

Merge branch 'dev' into osa

a403821

Bump the version

cd8e4e6

julienstroheker requested a review from troydai September 20, 2018 22:55

pep8

dda8819

troydai reviewed Sep 20, 2018

View reviewed changes

julienstroheker added 7 commits September 27, 2018 18:08

Update Unit Tests

a47fd2a

Add _ensure_osa_aad

lint

eeee1a6

pep8

e1f47af

Bump Version

2df28e2

Merge branch 'dev' of github.com:Azure/azure-cli into osa

6546358

# Conflicts: # src/command_modules/azure-cli-acs/HISTORY.rst # src/command_modules/azure-cli-acs/setup.py

Add tests

91961c1

add Vnet integration

04e4788

troydai reviewed Oct 1, 2018

View reviewed changes

julienstroheker added 5 commits October 3, 2018 11:51

Apply review

5f6b0dd

Add _remove_nulls_osa Update NetworkProfile

review

9d9240b

pep8

d9b2d41

Add the delegation on the OSA AAD object

f889de4

Add Preview Info

ac36154

Rename Models Rename node-count to compute-count fqdn mandatory

troydai requested a review from williexu October 16, 2018 01:50

fix aad default tenant

efaa154

julienstroheker requested a review from tjprescott October 24, 2018 20:20

42B reviewed Oct 28, 2018

View reviewed changes

tjprescott added do-not-merge ACS az acs/aks/openshift New Command(s) and removed ACS az acs/aks/openshift labels Oct 29, 2018

julienstroheker added 2 commits November 6, 2018 21:00

update api version for OSA operations

73ae6ec

Match Default VMSize for Master and Infra nodes

3b600a8

Rename node-vm-size to compute-vm-size

julienstroheker added 2 commits November 26, 2018 13:02

rename compute vm size

0fa3d39

Bump OpenShift version to 3.11

b7156ee

tjprescott added the closing soon Issues/PRs flagged to be closed soon due to inactivty, inapplicability, etc. label Dec 11, 2018

tjprescott closed this Dec 13, 2018

haroldrandom added closing soon Issues/PRs flagged to be closed soon due to inactivty, inapplicability, etc. do-not-merge New Command(s) labels Oct 25, 2019

		' The available locations are "{}"'.format(','.join(aci_locations)))


		def openshift_create(cmd, client, resource_group_name, name, # pylint: disable=too-many-locals

Conversation

julienstroheker commented Sep 20, 2018

Uh oh!

troydai 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!

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

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!

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!

troydai commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjprescott commented Dec 13, 2018

Uh oh!

julienstroheker commented Dec 13, 2018

Uh oh!

tjprescott commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

julienstroheker Sep 24, 2018 •

edited

Loading