Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Azure CLI Extension - Azure Arc Multicloud Connector #7954

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

alexmahonic
Copy link
Member

@alexmahonic alexmahonic commented Sep 5, 2024


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

Related command

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? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

Copy link

Validation for Breaking Change Starting...

Thanks for your contribution!

Copy link

Hi @alexmahonic,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 5, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

Copy link

github-actions bot commented Sep 5, 2024

CodeGen Tools Feedback Collection

Thank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey

Copy link

github-actions bot commented Sep 5, 2024

@alexmahonic alexmahonic force-pushed the feature-arc-connector branch 2 times, most recently from a45c7cd to 93fbd2e Compare September 13, 2024 18:04
@alexmahonic
Copy link
Member Author

@alexmahonic please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@alexmahonic alexmahonic marked this pull request as ready for review September 13, 2024 18:09
@kairu-ms kairu-ms merged commit c47980b into Azure:main Sep 18, 2024
20 checks passed
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ multicloud-connector ] : https://dev.azure.com/azclitools/release/_build/results?buildId=190376&view=results

Release History
===============

1.0.0b1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing 1.0.0.b1 is a beta version.
For GA, we should get rid of the beta version.

===============

1.0.0b1
++++++
Copy link

@bagajjal bagajjal Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

az arc-multicloud generate-aws-template command,

  1. output is of no use for customer consumption. It's not a valid aws cloudformation template.
    From the response body, remove the "body:" json element to make it valid aws cloudformation template.
    image

  2. Register the required RPs (Microsoft.AwsConnector, Microsoft.HybridConnectivity, Microsoft.HybridCompute). This is a global comment for all the CLI commands.

  3. Write the aws cloudformation template to a file named aws-cft-<customer-inputted-connector-name> and output the file on to the console. Ex - If the connector name is 264617844946 then the filename should be aws-cft-264617844946.json

  4. Add a new parameter --output-directory, used for writing the aws cloudformation template.

  5. remove the --solution-types parameter to avoid unnecessary confusion to customers. Help message doesn't show what should be provided in the --solution-types parameter. This was added mainly for the portal onboard flow. For CLI, we can assume that it's not supported scenario.
    Also, the help is not useful.
    image

  6. The examples should be more meaningful rather than some random strings,
    image


1.0.0b1
++++++
* Initial release.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test-permission command,

  1. The output {} doesn't make sense.

image

  1. It's NOT working.

CLI output
image

Portal output
image

  1. It's unclear what does --ids represent. Is it the connector Id or something else. Please treat this as a global comment as it's there in all the commands.

image

  1. It's unclear what --no-wait does. I don't see any difference of behavior with "0 or 1". Looks like the CLI command is not executing as I don't see the response {}

image

@@ -0,0 +1,8 @@
.. :changelog:

Copy link

@bagajjal bagajjal Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global comments applicable to all the CLI commands

  1. Register the required RPs (Microsoft.AwsConnector, Microsoft.HybridConnectivity, Microsoft.HybridCompute).

  2. Examples in the help message have some random characters. Please it to some meaningful names.
    image

  3. The error is printed twice. This is annoying if the error message is spanned across multiple lines,
    image

image

  1. The "--ids" argument have a generic text, it's unclear what it represents.

image

image

  1. The help should be very clear and should also point to our public documentation page that capture various options.


1.0.0b1
++++++
* Initial release.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

az arc-multicloud public-cloud-connector list command

  1. Command says list connector resources by subscriptionId but there is no argument to provide the subscriptionId. Interestingly there is an argument for resourcegroup. This seems odd.
    image

  2. -o table prints weird resource group names like clitest.rgy7xqoupodpyivrat6cuddih5wgnzd5rey6tir2xophpvkqckipflrawo2wred7tvs, looks like a formatting issue. can we suppress the global options?

image


1.0.0b1
++++++
* Initial release.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

az arc-multicloud public-cloud-connector update command,

  1. Exception rather than an error message. It would be hard for customers to understand stdout exception message, (ObjectMissingRequiredProperty) Missing required property: awsCloudProfile. Paths in payload: '$.properties.awsCloudProfile'

image

  1. connector is created as per the example, but the response is wrong as the awsCloudProfile is empty. This is also a bug on our controller. Please file a bug to fix the controller.
    Help message
    image

Connector Response
image

  1. As an end user, it's very hard to understand what should go into the aws-cloud-profile. The help should be very clear and should also point to our public documentation page that capture various options.
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants