-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Added support for retry policy update for failed items #11359
Conversation
Can one of the admins verify this patch? |
7ce9ef4
to
6574265
Compare
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.
Hi @sambitratha , please see my inline comments. Thanks!
[Parameter(Position = 1, Mandatory = true, HelpMessage = ParamHelpMsgs.Policy.ProtectionPolicy, | ||
ValueFromPipeline = true)] | ||
ValueFromPipeline = true, ParameterSetName = ModifyPolicyParamSet)] | ||
[Parameter(Position = 1, Mandatory = true, HelpMessage = ParamHelpMsgs.Policy.ProtectionPolicy, | ||
ValueFromPipeline = true, ParameterSetName = FixInconsistentPolicyParamSet)] |
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.
If the parameter appears in all the parameter sets, you don't have to specify it twice. In fact, the attribute can remain unchanged.
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.
In that case can i remove both parameter sets and instead just write Parameter(mandatory=true) ?
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.
And if it's allowed to do so, making changes in existing parametersets, will it be considered as a breaking 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.
Yes, by omitting the ParameterSetName = ***
you are telling powershell that every parameter sets owns it.
It's not changing existing param set, so is not a breaking change.
/// <summary> | ||
/// Retry Policy Update for Failed Items | ||
/// </summary> | ||
[Parameter(Mandatory = false, HelpMessage = ParamHelpMsgs.Policy.FixForInConsistentItems, |
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 think this switch parameter is mandatory in this parameter set, right?
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.
And ValidateNotNullOrEmpty
won't be necessary
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.
Besides, since you are changing the cmdlet surface, please regenerate help document.
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, will do the changes
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.
Looks good to me!
Added support for retry policy update for failed items
Added a parameter to specify whether or not to retry policy update for failed items.
Checklist
CONTRIBUTING.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added