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

Jonathanhe/workload network #3910

Merged
merged 33 commits into from
Oct 12, 2021

Conversation

jonathanhe-msft
Copy link
Contributor


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:

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.

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 23, 2021

vmware

Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

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

Please change one argument that I commented on, then this can be moved out of draft.

I'm not sure how several of these -id arguments should be handled. I'll have to differ to the CLI team reviewer. The guidelines recommend:

ID Arguments:
Arguments that end in -id should be GUIDs.
Arguments that expect ARM IDs should omit -id but call out that an ARM ID is allowed in the help text. These are often used as part of the "name or ID" convention (i.e: --storage-account can often accept a storage account name OR ARM ID).
https://github.com/Azure/azure-cli/blob/dev/doc/command_guidelines.md#argument-naming-conventions

Since these strings are not GUIDs, should the -id suffix's be removed?

src/vmware/azext_vmware/_help.py Outdated Show resolved Hide resolved
@jonathanhe-msft jonathanhe-msft marked this pull request as ready for review September 29, 2021 15:04
@cataggar
Copy link
Member

cataggar commented Oct 5, 2021

@zhoxing-ms, can you please review? See my earlier questions about the ids.

Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

Please change one argument that I commented on, then this can be moved out of draft.

I'm not sure how several of these -id arguments should be handled. I'll have to differ to the CLI team reviewer. The guidelines recommend:

ID Arguments:
Arguments that end in -id should be GUIDs.
Arguments that expect ARM IDs should omit -id but call out that an ARM ID is allowed in the help text. These are often used as part of the "name or ID" convention (i.e: --storage-account can often accept a storage account name OR ARM ID).
https://github.com/Azure/azure-cli/blob/dev/doc/command_guidelines.md#argument-naming-conventions

Since these strings are not GUIDs, should the -id suffix's be removed?

That's a good point! If they are not GUIDs, please remove the suffix of -id, otherwise users will be confused about the input value

@zhoxing-ms
Copy link
Contributor

@jonathanhe-msft Could you please fix those CI issues?

@jonathanhe-msft
Copy link
Contributor Author

@jonathanhe-msft Could you please fix those CI issues?

@zhoxing-ms All CI issues have been fixed

@zhoxing-ms
Copy link
Contributor

It looks good overall, but I have one small question: The file recording the release note is usually HISTORY.rst, and this one is CHANGELOG.md. May I ask can CHANGELOG.md be modified to HISTORY.rst?

@cataggar
Copy link
Member

but I have one small question: The file recording the release note is usually HISTORY.rst, and this one is CHANGELOG.md. May I ask can CHANGELOG.md be modified to HISTORY.rst?

Please, let's not change that. A CHANGELOG.md is commonly used in Python long descriptions too. It reads better on Github.

with open('CHANGELOG.md', encoding='utf-8') as f:
changelog = f.read()
setup(
name='vmware',
version=VERSION,
description='Azure VMware Solution commands.',
long_description=readme + '\n\n' + changelog,
long_description_content_type='text/markdown',

@zhoxing-ms zhoxing-ms merged commit a25ce5c into Azure:main Oct 12, 2021
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Oct 12, 2021

@jonathanhe-msft @cataggar In fact, we can modify the file name referenced in setup.py, such as code link

with open1('HISTORY.md', 'r', encoding='utf-8') as f:
    HISTORY = f.read()

For readability, we can only change the file name to HISTORY.md, and the file type can still be mark down. In this way, it can be consistent with the release notes of other extensions.
This is my personal suggestion. If you think it can be adopted, please submit a new PR to modify it, thanks

cataggar added a commit to cataggar/azure-cli-extensions that referenced this pull request Oct 20, 2021
zhoxing-ms pushed a commit that referenced this pull request Oct 26, 2021
* rename to HISTORY.md
#3910 (comment)

* publish vmware 3.2.0
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.

4 participants