-
Notifications
You must be signed in to change notification settings - Fork 21
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
Feat: CPF Merge #703
Feat: CPF Merge #703
Conversation
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.
@isc-shuliu I know this is still a draft but was curious so took a look and left a few comments
tests/integration_tests/Test/PM/Integration/_data/cpf-merge/Merge.cpf
Outdated
Show resolved
Hide resolved
HSDevOps was discussing today and along the lines of #633, we want to make sure that it is possible for HS to merge CPF resources only during deployment (and not during build-time). In both cases, we call zpm "load" or zpm "install", which means in both cases the initialize phase would run and merge the file. We had spoken about a CustomScope attribute in #633 but another option is to support just running the merge during a CustomPhase. @isc-shuliu @isc-tleavitt Thoughts on supporting this instead of adding the initialize lifecycle phase? And my apologies for not thinking of this before you implemented it, Shuheng! Update after Keshav reminded me of an important use case: we still want the CPF merge file to be merged by default during the initialize phase but would also like to additionally support it being merged during a custom phase. |
@isc-eneil
|
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.
Move of loading /preload in initialize instead of reload should be noted as a possible compatibility-breaking change (really would be an edge case). But I think this looks good otherwise.
@isc-tleavitt When would it break compatibility? Is it only when running |
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.
@isc-shuliu Looks great! Few questions/comments
@isc-shuliu I left just a couple of comments but it looks good to me too! |
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.
@isc-shuliu few more small comments/thoughts
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.
@isc-shuliu one very minor comment. Approving
If $IsObject(tProcessor) && tProcessor.%Extends("%IPM.ResourceProcessor.CustomPhaseMixin") { | ||
Set cp = tProcessor.CustomPhase | ||
If cp '= "" { | ||
Set pPhases($$$lcase(cp)) = cp |
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.
@isc-shuliu Apologies for the late review on this.
Should there be an additional check to make sure the custom phase hasn't already been added (since an Invoke and a CPF resource could technically give the same custom phase)?
cc @isc-kiyer
Implement #631