-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
✨ Add go/v4-alpha base and migration guide from go/v3 to go/v4-alpha #3032
✨ Add go/v4-alpha base and migration guide from go/v3 to go/v4-alpha #3032
Conversation
d8b123f
to
59037cb
Compare
/hold cancel |
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.
Thx @camilamacedo86 !
Added some nits.
I will go for some practices following the docs to finish my review.
|
||
## Common changes | ||
|
||
- `go/v4-alpha` projects use Kustomize v4x (instead of v3x) |
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.
Nits:
use Kustomize v4.x.x
(instead of v3.x.x
)
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.
In this case is 4x because we can bump the version.
The breaking changes are only faced in the major bump from 3x to 4x
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.
That makes sense. 👍🏼
59037cb
to
57943a0
Compare
Hi @Kavinjsir, Thank you for the help. Mainly all suggestions are applied. |
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.
lgtm!
- no longer provide backwards compatible support with k8s versions < `1.16` | ||
|
||
<aside class="note"> | ||
<H1> TL;DR of the New `go/v4-alpha` Plugin </H1> |
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.
See that below contains another tldr
header with contents. Do we need this h1
block?
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.
All notes have that. It creates a title @Kavinjsir
p.resource = res | ||
|
||
// TODO: re-evaluate whether y/n input still makes sense. We should probably always | ||
// scaffold the resource and controller. |
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.
Agree to directly generate resources and controller code with asking users.
Assuming these are the common patterns to implement CRD operators.
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.
Hun, we need to remove this TODO.
It cames from the go v3 code as well.
We need to have the Y/N option. WHY?
We can reconcile a Kind that is not owned by the project.
That can be a core type (i.e Deployments) or an external/third-party one like a CRD created by other projects. See: #3082
Lets remove it in a follow up.
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.
Sounds good. Makes sense to leave options for users considering on such wider use cases.
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.
Nits on doc. Else looks good to me! 👍
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
40951dc
to
2e7ddf5
Compare
Thank you for the review @varshaprasad96 |
Co-authored-by: Varsha <[email protected]>
5202445
to
8fcc643
Compare
@varshaprasad96 @everettraven @Kavinjsir could you help us move with this one? |
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.
Looks good to me!
p.resource = res | ||
|
||
// TODO: re-evaluate whether y/n input still makes sense. We should probably always | ||
// scaffold the resource and controller. |
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.
Sounds good. Makes sense to leave options for users considering on such wider use cases.
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.
Just a few more nits in doc. After they are addressed its /lgtm from my end.
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
docs/book/src/migration/manually_migration_guide_gov3_to_gov4.md
Outdated
Show resolved
Hide resolved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, Kavinjsir, varshaprasad96 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: Varsha <[email protected]>
Co-authored-by: Varsha <[email protected]>
Co-authored-by: Varsha <[email protected]>
Co-authored-by: Varsha <[email protected]>
Co-authored-by: Varsha <[email protected]>
Moving forward with this one since we got enough reviews! |
Description
Motivation
1.16