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

[Az Eventgrid] Add new features for 2021-12-31 release #17608

Merged
merged 20 commits into from
May 6, 2022
Merged

[Az Eventgrid] Add new features for 2021-12-31 release #17608

merged 20 commits into from
May 6, 2022

Conversation

rohkuma-microsoft
Copy link
Contributor

@rohkuma-microsoft rohkuma-microsoft commented Mar 24, 2022

Description

New features as part of 2021-12-31 release.

Design :
https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/1115

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)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Please provide help markdown for new added cmdlets. Seems like the PR is in draft status. I set it as draft. Please feel free to set this PR as ready to review status

src/EventGrid/EventGrid/Models/PSSytemTopic.cs Outdated Show resolved Hide resolved
src/EventGrid/EventGrid/Models/PSSytemTopic.cs Outdated Show resolved Hide resolved
public string ResourceGroupName { get; set; }

[Parameter(Mandatory = true,
ValueFromPipelineByPropertyName = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a -Force parameter for remove cmdlet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@BethanyZhou BethanyZhou marked this pull request as draft March 25, 2022 02:49
Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

  1. Please change parameter form to singularity
  2. Please remove validate set for switch parameters.
  3. please add -Force for remove-* cmdlet
  4. Please keep the implementation align with design.

src/EventGrid/EventGrid/Domain/NewAzureEventGridDomain.cs Outdated Show resolved Hide resolved
src/EventGrid/EventGrid/Domain/NewAzureEventGridDomain.cs Outdated Show resolved Hide resolved
public class GetAzureRmEventGridSystemTopic : AzureEventGridCmdletBase
{
[Parameter(
Mandatory = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all parameters are optional? As we discussed before, in certain parameter set, some parameters should be required. Please confirm that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is for getting system topics for a given subscription id. SubscriptionId is doesn't need to be separately passed as we get that from context.

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Please check all MDs and fill placeholders.

# Get-AzEventGridSystemTopic

## SYNOPSIS
{{ Fill in the Synopsis }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fill all placeholders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

```

## DESCRIPTION
{{ Fill in the Description }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add description for cmdlets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

## EXAMPLES

### Example 1
```powershell
Copy link
Contributor

Choose a reason for hiding this comment

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

A good example should be a good start for customer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


## NOTES

## RELATED LINKS
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to have if we can provide relate links.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@BethanyZhou BethanyZhou marked this pull request as ready for review May 5, 2022 02:07
@BethanyZhou
Copy link
Contributor

For our pipeline checks, it said:

"Az.EventGrid","Microsoft.Azure.Commands.EventGrid.NewAzureEventGridSystemTopic","New-AzEventGridSystemTopic","1","8420","Parameter set 'SystemTopicNameParameterSet' of cmdlet 'New-AzEventGridSystemTopic' contains at least one parameter with a position larger than four, which is discouraged.","Limit the number of positional parameters in a single parameter set to four or fewer."

Please have a look.

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

Seems like you missed one comment: https://github.com/Azure/azure-powershell/pull/17608/files#r834912423, need a -Force parameter for remove cmdlet

[Cmdlet(
"Get",
ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "EventGridFullUrlForSystemTopicEventSubscription",
SupportsShouldProcess = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Get-* cmdlet should not support should process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -197,4 +196,5 @@ This cmdlet supports the common parameters: -Debug, -ErrorAction, -ErrorVariable
## NOTES

## RELATED LINKS

https://docs.microsoft.com/en-us/azure/event-grid/system-topics
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
https://docs.microsoft.com/en-us/azure/event-grid/system-topics

Default value: None
Accept pipeline input: False
Accept wildcard characters: False
```

### -PassThru
{{ Fill PassThru Description }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fill this description

@@ -367,4 +367,5 @@ This cmdlet supports the common parameters: -Debug, -ErrorAction, -ErrorVariable
## NOTES

## RELATED LINKS

https://docs.microsoft.com/en-us/azure/event-grid/system-topics
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
https://docs.microsoft.com/en-us/azure/event-grid/system-topics

```

## DESCRIPTION
{{ Fill in the Description }}
Copy link
Contributor

Choose a reason for hiding this comment

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

please have a look

Copy link
Contributor

@BethanyZhou BethanyZhou left a comment

Choose a reason for hiding this comment

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

LGTM

@BethanyZhou
Copy link
Contributor

/azp run azure-powershell - security-tools

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

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.

2 participants