[Spring Cloud] Add buildpack-binding command for Enterprise tier. by Incarnation-p-lee · Pull Request #4302 · Azure/azure-cli-extensions

Incarnation-p-lee · 2022-01-07T02:27:38Z

Add create, show, set, delete and list for buildpack-binding.
Add enable app insights when create Enterprise tier.

Signed-off-by: Pan Li panli@microsoft.com

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

General Guidelines

Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

* Add create, show, set, delete and list for buildpack-binding. * Add enable app insights when create Enterprise tier. Signed-off-by: Pan Li <panli@microsoft.com>

Incarnation-p-lee · 2022-01-07T02:28:28Z

The test part is under preparing, will be soon.

yonzhan · 2022-01-07T02:31:48Z

Spring-Cloud

…rprise

Signed-off-by: Pan Li <panli@microsoft.com>

wangzelin007 · 2022-01-07T04:25:35Z

src/spring-cloud/azext_spring_cloud/_validators_enterprise.py

+                                                         namespace.builder_name,
+                                                         namespace.name)
+        if binding_resource is not None:
+            raise CLIError('buildpack Binding {} in builder {} already exists '


Could you please use a specific error type instead of CLIError ?

wangzelin007 · 2022-01-07T04:26:37Z

src/spring-cloud/azext_spring_cloud/buildpack_binding.py

+                   'You can visit %s/#resource%s/overview to view your '
+                   'Application Insights component', appinsights.name, portal_url, appinsights.id)
+
+    return appinsights


Please add newline at end of file.

wangzelin007 · 2022-01-07T04:30:56Z

src/spring-cloud/azext_spring_cloud/_params.py

+                            'Format "key[=value]".',
+                       nargs='*',
+                       validator=validate_buildpack_binding_secrets)
+            c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_not_exist)


Suggested change

c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_not_exist)

c.argument('name', name_type, help='Name for buildpack binding.', validator=validate_buildpack_binding_not_exist)

wangzelin007 · 2022-01-07T04:31:13Z

src/spring-cloud/azext_spring_cloud/_params.py

+                            'Format "key[=value]".',
+                       nargs='*',
+                       validator=validate_buildpack_binding_secrets)
+            c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)


Suggested change

c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)

c.argument('name', name_type, help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)

wangzelin007 · 2022-01-07T04:31:24Z

src/spring-cloud/azext_spring_cloud/_params.py

+    for scope in ['spring-cloud build-service builder buildpack-binding show',
+                  'spring-cloud build-service builder buildpack-binding delete']:
+        with self.argument_context(scope) as c:
+            c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)


Suggested change

c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)

c.argument('name', name_type, help='Name for buildpack binding.', validator=validate_buildpack_binding_exist)

wangzelin007 · 2022-01-07T04:42:34Z

src/spring-cloud/azext_spring_cloud/_params.py

+        c.argument('app', app_name_type, help='Name of app.', validator=validate_app_name)
+
+    with self.argument_context('spring-cloud service-registry unbind') as c:
+        c.argument('app', app_name_type, help='Name of app.', validator=validate_app_name)


Duplicate with line 437, please delete line 429-433

wangzelin007 · 2022-01-07T04:45:58Z

src/spring-cloud/azext_spring_cloud/_params.py

+
+    for scope in ['spring-cloud build-service builder buildpack-binding show',
+                  'spring-cloud build-service builder buildpack-binding delete']:
+        with self.argument_context(scope) as c:


Suggested change

with self.argument_context(scope) as c:

for scope in ['show', 'delete']:

with self.argument_context('spring-cloud build-service builder buildpack-binding {}'.format(scope)) as c:

wangzelin007 · 2022-01-07T04:50:31Z

src/spring-cloud/azext_spring_cloud/_params.py

+            c.argument('service', service_name_type, validator=only_support_enterprise)
+
+
+    for scope in ['spring-cloud build-service builder buildpack-binding set']:


It's better to merge spring-cloud build-service builder buildpack-binding create with set except the argument name.
Otherwise we don't need to use scope here.

One question, is this the convention to merge such kinds of statements? If so, I can align this.
As a beginner to CLI, I intended to write parms like this, because it is clean when I look into one command for parameters. I can have a full picture of all parameters of one command.
Compare to merging, I may need to combine these in my mind for full picture.

Could you please help to enlighten me what is the best practice here?

Hi Li,
We highly recommend merging, because after merging, the similarities and differences can be more clearly distinguished.
If you modify it later，it is not easy to miss anything after merge.

sure, updated.

Incarnation-p-lee · 2022-01-07T05:32:27Z

#4294

Signed-off-by: Pan Li <panli@microsoft.com>

Incarnation-p-lee · 2022-01-07T12:05:17Z

@wangzelin007 test added.

Signed-off-by: Pan Li <panli@microsoft.com>

zhoxing-ms · 2022-01-10T02:40:13Z

src/spring-cloud/azext_spring_cloud/_help.py

+helps['spring-cloud build-service builder buildpack-binding list'] = """
+    type: command
+    short-summary: (Support Enterprise Tier Only) List all buildpack binding in a builder. The secrets will be masked.
+    examples:
+        - name: Show a buildpack binding.
+          text: az spring-cloud build-service builder buildpack-binding list --builder-name first-builder --service MyCluster --resource-group MyResourceGroup
+"""


name: Show a buildpack binding.

Why is the description here Show rather than list?

Good catch, thank you!

zhoxing-ms · 2022-01-10T02:58:19Z

src/spring-cloud/azext_spring_cloud/_validators_enterprise.py

-
+from azure.cli.core.commands.validators import validate_tag
+from azure.core.exceptions import ResourceNotFoundError
+from azure.cli.core.util import CLIError


from azure.cli.core.util import CLIError

Please remove the useless import

zhoxing-ms · 2022-01-10T03:08:37Z

@Incarnation-p-lee Could you please fix the CI issues?

Signed-off-by: Pan Li <panli@microsoft.com>

Incarnation-p-lee · 2022-01-10T03:15:51Z

@zhoxing-ms Sure, will be soon.

…rprise

Signed-off-by: Pan Li <panli@microsoft.com>

zhoxing-ms · 2022-01-10T03:29:39Z

src/spring-cloud/azext_spring_cloud/commands.py

+        g.custom_command('set', 'create_or_update_buildpack_binding')
+        g.custom_show_command('show', 'buildpack_binding_show')
+        g.custom_command('list', 'buildpack_binding_list')
+        g.custom_command('delete', 'buildpack_binding_delete')


Do we need consider adding confirmation=True for the delete command?

make sense, let me update it.

Signed-off-by: Pan Li <panli@microsoft.com>

wangzelin007 · 2022-01-10T06:44:57Z

src/spring-cloud/azext_spring_cloud/tests/latest/test_asc_buildpack_binding.py

+            ])
+
+        results = self.cmd('spring-cloud build-service builder buildpack-binding list -g {rg} -s {serviceName}').get_output_in_json()
+        self.assertEqual(2, len(results))


Please add newline at end of file.

Signed-off-by: Pan Li <panli@microsoft.com>

[Spring-Cloud] Add buildpack-binding command for Enterprise tier.

dd89387

* Add create, show, set, delete and list for buildpack-binding. * Add enable app insights when create Enterprise tier. Signed-off-by: Pan Li <panli@microsoft.com>

yonzhan requested review from jsntcy, wangzelin007 and zhoxing-ms January 7, 2022 02:31

yonzhan assigned zhoxing-ms Jan 7, 2022

yonzhan added this to the Jan 2022 (2022-02-08) milestone Jan 7, 2022

Incarnation-p-lee and others added 7 commits January 7, 2022 10:41

Merge branch 'main' into panli/add-buildpack-binding-command-for-ente…

ba6ad2b

…rprise

Minor change.

e384b9b

Signed-off-by: Pan Li <panli@microsoft.com>

format.

b18fecc

Signed-off-by: Pan Li <panli@microsoft.com>

Make linter happy.

5226d73

Signed-off-by: Pan Li <panli@microsoft.com>

Reset help.py.

0a7aa27

Signed-off-by: Pan Li <panli@microsoft.com>

Re-add help.py

d015c88

Signed-off-by: Pan Li <panli@microsoft.com>

Add EOF.

9aed17d

Signed-off-by: Pan Li <panli@microsoft.com>

wangzelin007 reviewed Jan 7, 2022

View reviewed changes

wangzelin007 changed the title ~~[Spring-Cloud] Add buildpack-binding command for Enterprise tier.~~ [Spring Cloud] Add buildpack-binding command for Enterprise tier. Jan 7, 2022

wangzelin007 reviewed Jan 7, 2022

View reviewed changes

Incarnation-p-lee added 2 commits January 7, 2022 17:09

Add UT for create enterprise tier with app insights.

7941c6f

Signed-off-by: Pan Li <panli@microsoft.com>

Add record test for buildpack bindings.

7f3ed5d

Signed-off-by: Pan Li <panli@microsoft.com>

Incarnation-p-lee added 2 commits January 10, 2022 09:47

Resolve comments.

e689778

Signed-off-by: Pan Li <panli@microsoft.com>

Resolve comments.

135aa2c

Signed-off-by: Pan Li <panli@microsoft.com>

zhoxing-ms reviewed Jan 10, 2022

View reviewed changes

Resolve comments and make static check happy.

d374d52

Signed-off-by: Pan Li <panli@microsoft.com>

Incarnation-p-lee and others added 2 commits January 10, 2022 11:16

Merge branch 'main' into panli/add-buildpack-binding-command-for-ente…

f1683a1

…rprise

Make static check happy.

59d3690

Signed-off-by: Pan Li <panli@microsoft.com>

zhoxing-ms reviewed Jan 10, 2022

View reviewed changes

Incarnation-p-lee added 5 commits January 10, 2022 11:34

Resolve comments.

5211830

Signed-off-by: Pan Li <panli@microsoft.com>

Resolve comments.

bc47c30

Signed-off-by: Pan Li <panli@microsoft.com>

Replace CRLF to LF.

cfbcab3

Signed-off-by: Pan Li <panli@microsoft.com>

Minor change format.

20ed905

Signed-off-by: Pan Li <panli@microsoft.com>

Make static check happy.

627334d

Signed-off-by: Pan Li <panli@microsoft.com>

wangzelin007 reviewed Jan 10, 2022

View reviewed changes

Merge parmas.

818094c

Signed-off-by: Pan Li <panli@microsoft.com>

wangzelin007 approved these changes Jan 11, 2022

View reviewed changes

zhoxing-ms approved these changes Jan 11, 2022

View reviewed changes

zhoxing-ms merged commit f549964 into Azure:main Jan 11, 2022

Incarnation-p-lee deleted the panli/add-buildpack-binding-command-for-enterprise branch January 12, 2022 05:28

	c.argument('name', help='Name for buildpack binding.', validator=validate_buildpack_binding_not_exist)
	c.argument('name', name_type, help='Name for buildpack binding.', validator=validate_buildpack_binding_not_exist)

	with self.argument_context(scope) as c:
	for scope in ['show', 'delete']:
	with self.argument_context('spring-cloud build-service builder buildpack-binding {}'.format(scope)) as c:

		c.argument('service', service_name_type, validator=only_support_enterprise)


		for scope in ['spring-cloud build-service builder buildpack-binding set']:

Conversation

Incarnation-p-lee commented Jan 7, 2022

General Guidelines

About Extension Publish

Uh oh!

Incarnation-p-lee commented Jan 7, 2022

Uh oh!

yonzhan commented Jan 7, 2022

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!

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!

wangzelin007 Jan 7, 2022 • 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!

Incarnation-p-lee commented Jan 7, 2022

Uh oh!

Incarnation-p-lee commented Jan 7, 2022

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!

zhoxing-ms Jan 10, 2022 • 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!

zhoxing-ms commented Jan 10, 2022

Uh oh!

Incarnation-p-lee commented Jan 10, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

wangzelin007 Jan 7, 2022 •

edited

Loading

zhoxing-ms Jan 10, 2022 •

edited

Loading