Skip to content
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: Add spec.manager #1958

Merged
merged 11 commits into from
Oct 16, 2024
Merged

feat: Add spec.manager #1958

merged 11 commits into from
Oct 16, 2024

Conversation

ruanxin
Copy link
Contributor

@ruanxin ruanxin commented Oct 14, 2024

Description

Related issue(s)
#1840

@ruanxin ruanxin requested review from a team as code owners October 14, 2024 15:42
@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2024
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking a bit about the name as well. I think staying with manager is okay as we also describe that this is our primary expected case. Also, I still have my doubts if Cloud Manager will even have a use case for manual installation. So I would say it is okay. If we really find that the name is bad, I think we could also rename it in a future API version or even introduce a conversion webhook replacing it with another field name.

api/v1beta2/moduletemplate_types.go Show resolved Hide resolved
api/v1beta2/moduletemplate_types.go Outdated Show resolved Hide resolved
api/v1beta2/moduletemplate_types.go Outdated Show resolved Hide resolved
docs/contributor/resources/03-moduletemplate.md Outdated Show resolved Hide resolved
docs/contributor/resources/03-moduletemplate.md Outdated Show resolved Hide resolved
docs/contributor/resources/03-moduletemplate.md Outdated Show resolved Hide resolved
docs/contributor/resources/03-moduletemplate.md Outdated Show resolved Hide resolved
Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>
@kyma-bot kyma-bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 15, 2024
ruanxin and others added 4 commits October 15, 2024 13:56
Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>
Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>
Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>
@kyma-bot kyma-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2024
c-pius
c-pius previously approved these changes Oct 15, 2024
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last thought but also LGTM as is.

docs/contributor/resources/03-moduletemplate.md Outdated Show resolved Hide resolved
@kyma-bot kyma-bot added the lgtm Looks good to me! label Oct 15, 2024
Co-authored-by: Małgorzata Świeca <malgorzata.swieca@sap.com>
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Oct 16, 2024
ruanxin and others added 2 commits October 16, 2024 10:44
Co-authored-by: Małgorzata Świeca <malgorzata.swieca@sap.com>
Co-authored-by: Małgorzata Świeca <malgorzata.swieca@sap.com>
ruanxin and others added 2 commits October 16, 2024 11:15
@ruanxin ruanxin enabled auto-merge (squash) October 16, 2024 09:18
@kyma-bot kyma-bot added the lgtm Looks good to me! label Oct 16, 2024
@c-pius c-pius requested a review from mmitoraj October 16, 2024 09:58
@ruanxin ruanxin merged commit 5eddd74 into kyma-project:main Oct 16, 2024
60 of 61 checks passed
@c-pius c-pius assigned ruanxin and unassigned c-pius and mmitoraj Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants