Fix Issue 5896 #6004
Fix Issue 5896 #6004
Conversation
markcowl
left a comment
markcowl
left a comment
There was a problem hiding this comment.
I think we should also add to the job cmdlet tests to verify these properties are copied.
| { | ||
| get | ||
| { | ||
| if (_asJobSessionState == null) |
There was a problem hiding this comment.
I don't think we can instantiate this lazily, because it might be first referenced inside a job - I think we need to instantiate it either during BeginProcessing. or in the cmdlet constructor
| } | ||
| } | ||
|
|
||
| private RuntimeDefinedParameterDictionary _copiedDynamicParameters; |
There was a problem hiding this comment.
How are we assignign a value to this?
There was a problem hiding this comment.
We assign it once the dynamic parameters are created in the cmdlet - there is no base property for DynamicParameters, and they can be changed after the AzurePSCmdlet is instantiated.
|
@azuresdkci Test this please |
| if (_copiedDynamicParameters == null) | ||
| { | ||
| _copiedDynamicParameters = new RuntimeDefinedParameterDictionary(); | ||
| return _copiedDynamicParameters; |
There was a problem hiding this comment.
this extra return is unnecessary
| MyInvocation.MyCommand.Parameters.Keys.ToArray()); | ||
| } | ||
| } | ||
| this.CopiedDynamicParameters = dynamicParameters; |
There was a problem hiding this comment.
Hmm, this isn't the interface I was hoping for. Is this the only place we can get DynamicParameters from?
If we need to do this, I think a void method with a bit less esoteric name, like ```void RegisterDynamicParameters(RuntimeParamaterDictionary paremeters) might be a more intuitive option for new usages.
There was a problem hiding this comment.
Unfortunately I could not find any way to get the dynamic parameters outside of copying them over during runtime. I created the void method and updated the relevant files.
| this.dynamicParameters.Add(param.Name, dp); | ||
| } | ||
| } | ||
| this.CopiedDynamicParameters = dynamicParameters; |
There was a problem hiding this comment.
extra vertical space after '}'
|
|
||
| private SessionState _asJobSessionState; | ||
|
|
||
| public SessionState AsJobSessionState |
There was a problem hiding this comment.
I wonder if we shouldn't Hide SessionState, and throw if it is used, thoughts?
There was a problem hiding this comment.
In fact, I'm wondering if my previous advice about not hiding was good. If we can get the propertes using base., then hiding them would make the interface simpler.
Otherwise, we should just rething the name of CopiedParameterDictionary (maybe AsJobParameterDictionary)
There was a problem hiding this comment.
I think it makes sense to hide it here - updated accordingly.
4 participants
Description
#5896
Checklist
CONTRIBUTING.mdplatyPSmodule