-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[Monitor] Fixing issue #3957: bug in Get-AzureRmLog cmdlet, returning… #4066
Conversation
…rning at most 200 records despite of MaxEvents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder can you add a test to verify this bug fix?
[ValidateNotNullOrEmpty] | ||
public virtual int MaxEvents { get; set; } | ||
public virtual int MaxRecords { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder any reason for the parameter name change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a test for this, and need to update the changelog.md to describe the fix
…AzureRmLog cmdlet.
@gucalder Hey Guillermo, is this PR ready for review? |
…d. Fixing some documentation issue in the GetAzureRmLogCommand.cs.
Now it should be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder Hey Guillermo, just a couple of quick clarifying questions
this.MaxEvents = value; | ||
} | ||
// If value is not acceptable this forces the use of the default value | ||
this.MaxRecords = (value > 0 && value <= 100000) ? value : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder in the change log, you mentioned
Any value for MaxEvents that is less than 1 is ignored and the default is used instead
and that the default value is 1000. However, here you are setting the value to 0. If this is setting something different, ignore this comment 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is odd.
There are historic reasons to do it that way. They have to do with the fact that this part of the code is shared by two other cmdlets where MaxEvents is not defined as a parameter, and also MaxEvents was added later.
Anyway this is one thing that I might change later to make it a bit less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder just to make sure I understand, does the MaxEvent
value get set to 1000 somewhere else (by default), or is the description in the change log incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, MaxEvent is assigned 1000 somewhere else.
.SYNOPSIS | ||
Tests getting the logs for a subscription Id. | ||
#> | ||
function Test-GetAzureSubscriptionIdLogMaxEvents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gucalder are these the tests containing the pages of size 10 that return 15 elements? (the test you mentioned in the email)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is one of the places where this is tested.
This is another scenario test for that. NOTE: the page here is set to 6 records, but the are 8 records in total.
Test-GetAzureSubscriptionIdLogPaged (same file as Test-GetAzureSubscriptionIdLogMaxEvents)
This is another place, the unit test below. Here the first page is 10 records and the second 5.
GetAzureSubscriptionIdLogCommandParametersProcessing()
I added more comments to clarify the goal of the individual tests and also added more cases to the existing scenario tests.
…their goal and adding some more cases to the tests.
Tests and change log have been added
#4056
Description
This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThru
parameter.Cmdlet Parameter Guidelines