Fix: IPM Update Resource Conflict#1129
Conversation
isc-jlechtne
requested review from
isc-cborbonm,
isc-dchui,
isc-egabhart,
isc-eneil,
isc-jili,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-cborbonm
left a comment
isc-cborbonm
left a comment
There was a problem hiding this comment.
just left a couple of comments! but it looks great to me 😃
| @@ -1371,6 +1364,12 @@ Method GetResolvedReferences( | |||
| } | |||
|
|
|||
| if pLockedDependencies { | |||
There was a problem hiding this comment.
question: why did this get moved to only when pLockedDependencies is set?
There was a problem hiding this comment.
pDependencyGraph is only used within this if-statement. Felt unnecessary to me to build the dependency graph earlier if the execution never got to the point where we actually use it
| /// See property description for more details on the usage of MarkedForUpdate | ||
| ClassMethod MarkResourceForUpdate( | ||
| resourceName As %String, | ||
| marking As %Boolean = 1) |
There was a problem hiding this comment.
is there a use case for this method when marking would be set to 0?
There was a problem hiding this comment.
Not currently but for what the method is doing I felt it made sense to have the capability work both ways.
|
@isc-jlechtne mind adding your test cases to the PR description? Thanks! |
isc-dchui
left a comment
isc-dchui
left a comment
There was a problem hiding this comment.
The only thing not already mentioned is that "addresses" is not actually one of the magic github closing words so won't auto-link the issue. You'll need "fixes" or "resolves". Also more detail in the description would be nice!
isc-cborbonm
left a comment
isc-cborbonm
left a comment
There was a problem hiding this comment.
looks good to me :)
4 participants
Fixes #1128
Updates update test with the following resources:
module-a(Class1.cls) andmodule-b(Class2.cls)Class1.clsis inmodule-candClass2.clsis inmodule-amodule-a v2.0.5+snapshotwithClass1.clsSteps added:
module-a v2.0.5+snapshot. This will fail due toClass1.clsbelonging to bothmodule-aandmodule-cmodule-a v2.1.0+snapshothasClass1andClass2moving around modules, which will no longer fail due to this fix