Skip to content

Conversation

@MehaKaushik
Copy link
Contributor

@MehaKaushik MehaKaushik commented Dec 17, 2019

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@adxsdkps
Copy link
Collaborator

Can one of the admins verify this patch?

@msJinLei msJinLei self-assigned this Dec 17, 2019
@msJinLei
Copy link
Contributor

Please correct the mandatory property of the parameters. For NameParameterSet, name related parameters should be mandatory and for InputObjectParameterSet at least InputObject is mandatory. If correct all the cases.

@MehaKaushik MehaKaushik changed the title Cmdlets for CosmosDB Sql Operations Cmdlets for CosmosDB Sql and Account Operations Dec 18, 2019
@MehaKaushik
Copy link
Contributor Author

@msJinLei Please review at your earliest convenience. Addressed your previous comments.

@msJinLei
Copy link
Contributor

msJinLei commented Dec 25, 2019

@MehaKaushik Please remove the modification of other modules.

@MehaKaushik
Copy link
Contributor Author

@msJinLei Made the changes as per your request, no other modules are being shown in the commit history now.

Copy link
Contributor

@msJinLei msJinLei left a comment

Choose a reason for hiding this comment

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

  • If the cmdlet supports "shouldprocess", please call ShouldProcess in your method.
  • For PassThru output, you can write bool to pipeline directly. No need to transform to string.
  • Generally, it is a little strange to return error message as an output. You can WriteError and return false.
  • PassThru is only required for Remove cmdlets, please return object modified in the New & Set & Update ones
  • AsJob means the cmdlet costs a long time to run. Please double confirm all of your “AsJob"s are required.
  • Please also involve one of your team members for logic integrity review.

@MehaKaushik
Copy link
Contributor Author

MehaKaushik commented Dec 27, 2019

@msJinLei Thanks for your review. I have addressed all of the comments. Please have another look.

@MehaKaushik
Copy link
Contributor Author

@msJinLei Please review at your earliest convenience.

@msJinLei
Copy link
Contributor

msJinLei commented Jan 2, 2020

@msJinLei Please review at your earliest convenience.

OK. I am working on fixing some urgent issues these days. And will review your PR when I have time.

@msJinLei
Copy link
Contributor

msJinLei commented Jan 8, 2020

@MehaKaushik

  • Please modify your output object
    • for New- & Update- & Set- cmdlets, please return the object you operate on
    • for Remote-, please return bool type.

https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/design-guidelines/cmdlet-best-practices.md#output-type

@msJinLei msJinLei merged commit ef61e22 into Azure:master Jan 17, 2020
dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020
Cmdlets for CosmosDB Sql and Account Operations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants