Skip to content

Feat: CPF Merge - #703

Merged
isc-shuliu merged 36 commits into
v0.10.xfrom
v0.10.x-feat-init-cpf-merge
Jan 29, 2025
Merged

Feat: CPF Merge#703
isc-shuliu merged 36 commits into
v0.10.xfrom
v0.10.x-feat-init-cpf-merge

Conversation

@isc-shuliu

Copy link
Copy Markdown
Contributor

Implement #631

@isc-kiyer isc-kiyer left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/Lifecycle/Base.cls Outdated
Comment thread src/cls/IPM/Lifecycle/Base.cls Outdated
Comment thread src/cls/IPM/Lifecycle/Base.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread tests/integration_tests/Test/PM/Integration/CPFMerge.cls
Comment thread tests/integration_tests/Test/PM/Integration/_data/cpf-merge/Merge.cpf Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/Lifecycle/Base.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread tests/integration_tests/Test/PM/Integration/CPFMerge.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
@isc-shuliu
isc-shuliu marked this pull request as ready for review January 15, 2025 23:17
@isc-eneil

isc-eneil commented Jan 16, 2025

Copy link
Copy Markdown
Collaborator

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.

cc @isc-kiyer @isc-jlechtne @isc-jili

@isc-shuliu

isc-shuliu commented Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

@isc-eneil
In that case, shall we do this

  • <CPF Name="merge.cpf"> will by default merge the CPF during the new Initialize phase,
  • <CPF Name="merge.cpf" Phase="MakeDeployed" When="Before/After"> will merge the CPF before/after the MakeDeployed phase. The phase name is customizable.
  • <CPF Name="merge.cpf" CustomPhase="MyPhaseName"> will merge the CPF during the MyPhaseName custom phase

@isc-tleavitt isc-tleavitt left a comment

Copy link
Copy Markdown
Contributor

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-shuliu

Copy link
Copy Markdown
Contributor Author

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 <package> reload -only?

@isc-kiyer isc-kiyer left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/cls/IPM/DataType/PhaseName.cls Outdated
Comment thread src/cls/IPM/DataType/ResourceDirectory.cls
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
Comment thread src/cls/IPM/Storage/Module.cls Outdated
@isc-eneil

Copy link
Copy Markdown
Collaborator

@isc-shuliu I left just a couple of comments but it looks good to me too!

@isc-shuliu isc-shuliu changed the title V0.10.x feat init cpf merge Feat: CPF Merge Jan 22, 2025

@isc-kiyer isc-kiyer left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/cls/IPM/Storage/Module.cls Outdated
Comment thread src/cls/IPM/ResourceProcessor/Abstract.cls Outdated
Comment thread src/cls/IPM/Storage/Module.cls Outdated

@isc-kiyer isc-kiyer left a comment

Copy link
Copy Markdown
Collaborator

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

Comment thread src/cls/IPM/ResourceProcessor/CPF.cls Outdated
@isc-shuliu
isc-shuliu merged commit f2c631f into v0.10.x Jan 29, 2025
@isc-shuliu
isc-shuliu deleted the v0.10.x-feat-init-cpf-merge branch January 29, 2025 16:51
If $IsObject(tProcessor) && tProcessor.%Extends("%IPM.ResourceProcessor.CustomPhaseMixin") {
Set cp = tProcessor.CustomPhase
If cp '= "" {
Set pPhases($$$lcase(cp)) = cp

@isc-eneil isc-eneil Jan 29, 2025

Copy link
Copy Markdown
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for performing module-specified namespace or system configuration at installation/load time

4 participants