Skip to content

Small documentation change to Get-AzLog.md#11068

Merged
VeryEarly merged 8 commits intoAzure:masterfrom
theheatDK:patch-10
Feb 11, 2020
Merged

Small documentation change to Get-AzLog.md#11068
VeryEarly merged 8 commits intoAzure:masterfrom
theheatDK:patch-10

Conversation

@theheatDK
Copy link
Copy Markdown
Contributor

@theheatDK theheatDK commented Feb 9, 2020

The -CorrelationId parameter is not a required parameter.

It is not mentioned anywhere that the module retrieves Activity Log events. It is nice with a bit of context. In the Portal it is called Activity Log and the CLI uses "az monitor activity-log".

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

The -CorrelationId parameter is not a required parameter.
@theheatDK theheatDK changed the title Update Get-AzLog.md Small documentation change to Get-AzLog.md Feb 9, 2020

### -CorrelationId
Specifies the correlation ID.
This parameter is required.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this parameter seems required
Mandatory = true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But it is not. Look at the examples.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you point which example indicates "-CorrelationId" is not required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Get-AzLog -MaxRecord 100
Get-AzLog -ResourceId $ResourceId -MaxRecord 5
Get-AzLog -DetailedOutput -ResourceId $ResourceId -Caller '*@dnhf.onmicrosoft.com'

All of these works fine.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

cmdlets can have multiple parameter sets,
CorrelationId is required in parameter set "CorrelationIdParameterSetName", other parameter sets could work fine without it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, this is the last time I will ever make any contribution to the Azure Powershell module.
I waste my weekend trying to make it better and it is just closed!! No wonder the documentation is so shit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @theheatDK ,
this PR is to make change the description of parameter "-CorrelationId" which is incorrect as we discussed.

We appreciate your passion and effort to help make azure-powershell better, however to have an alias for this cmdlet, we need a dedicated issue to track it.

About this help doc, it seems it was not generated properly, thanks for point it out, we'll regenerate doc for monitor in the future releases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wrote this:

The -CorrelationId parameter is not a required parameter.

It is not mentioned anywhere that the module retrieves Activity Log events. It is nice with a bit of context. In the Portal it is called Activity Log and the CLI uses "az monitor activity-log".

I made 3 changes to the text. One regarding the parameter and two to make it clear that this cmdlet is used for Activity logs. I would have assumed that you looked at my changes before dismissing them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reopened PR,
Can you add the "This parameter is required." back, and something more meaningful in the ChangeLog.md? Something like "fix description of cmdlet, thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @VeryEarly,
I have added the text back and changed the text in the ChangeLog.md.
Regards Peter

@VeryEarly
Copy link
Copy Markdown
Collaborator

It's true that the name is inconsistent with CLI and azure portal.
Cmdlet name cannot be changed until next breaking change release, we can add alias to it.

VeryEarly
VeryEarly previously approved these changes Feb 11, 2020
@VeryEarly
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@VeryEarly VeryEarly merged commit 57c3343 into Azure:master Feb 11, 2020
@theheatDK theheatDK deleted the patch-10 branch February 11, 2020 05:48
dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020
Small documentation change to Get-AzLog.md
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.

2 participants