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

✨ RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade implementation #6608

Conversation

ykakarap
Copy link
Contributor

@ykakarap ykakarap commented Jun 8, 2022

What this PR does / why we need it:

This implements 2 non-tracking lifecycle-hooks:

  • BeforeClusterCreate
  • BeforeClusterUpgrade

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #6546

/area runtime-sdk

@k8s-ci-robot k8s-ci-robot added area/runtime-sdk Issues or PRs related to Runtime SDK cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 8, 2022
@ykakarap ykakarap changed the title 🌱 RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation 🌱 [WIP] RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation Jun 8, 2022
@ykakarap
Copy link
Contributor Author

ykakarap commented Jun 8, 2022

/hold

This PR should be merged after #6581 is merged.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 5897927 to 522e82c Compare June 8, 2022 06:53
@sbueringer
Copy link
Member

sbueringer commented Jun 8, 2022

/hold cancel
underlying PR is merged, needs a rebase though :) (but Prow will block for that anyway)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 8, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 38702fe to bca1040 Compare June 8, 2022 18:13
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 8, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from bca1040 to 3ecb5e8 Compare June 8, 2022 18:14
@ykakarap ykakarap changed the title 🌱 [WIP] RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation 🌱 RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation Jun 8, 2022
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Very nice, looks a lot more straightforward then what I thought (I know this PR doesn't contain the tracking stuff yet :))

I think the only bigger discussion we have is: #6608 (comment) Everything else is just details.

internal/controllers/cluster/cluster_controller_test.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/scope/scope.go Outdated Show resolved Hide resolved
internal/runtime/client/test/fake_client.go Outdated Show resolved Hide resolved
internal/controllers/topology/cluster/desired_state.go Outdated Show resolved Hide resolved
@ykakarap ykakarap changed the title 🌱 RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation ✨ RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation Jun 9, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from f05f8f8 to ce026ad Compare June 9, 2022 20:03
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Looks good a few nits, otherwise lgtm from my side

/assign @fabriziopandini
Please take a look already when you have some time

internal/controllers/cluster/cluster_controller_test.go Outdated Show resolved Hide resolved
internal/controllers/cluster/cluster_controller.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from acbeed8 to 5011418 Compare June 10, 2022 19:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2022
@ykakarap ykakarap changed the title ✨ RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade, BeforeClusterDelete implementation ✨ RuntimeSDK: BeforeClusterCreate, BeforeClusterUpgrade implementation Jun 12, 2022
@sbueringer
Copy link
Member

Thank you very much!

/lgtm
/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022
@enxebre
Copy link
Member

enxebre commented Jun 13, 2022

folks any chance we can include the issue this solves in the PR desc so it's easier to navigate across all this interrelated work?

@sbueringer
Copy link
Member

sbueringer commented Jun 13, 2022

folks any chance we can include the issue this solves in the PR desc so it's easier to navigate across all this interrelated work?

Done (also on the other one). I agree we should include some kind of fixes / part of / ... . We tried to do this in the Runtime SDK PRs in general, I suppose we forgot it here.

@ykakarap
Copy link
Contributor Author

I forgot adding the issue here. Thanks @sbueringer for adding it to the other as well. :)

@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 5011418 to 1739897 Compare June 13, 2022 16:07
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 1739897 to 9b9187d Compare June 13, 2022 16:34
@sbueringer
Copy link
Member

lgtm pending squash

@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 9b9187d to 34235b0 Compare June 13, 2022 16:44
@ykakarap
Copy link
Contributor Author

lgtm pending squash

squshed

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Great work! Things are really shaping out nicely

main.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 13, 2022
@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 04d3b9e to 6af1ea5 Compare June 13, 2022 18:32
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2022
@fabriziopandini
Copy link
Member

lgtm pending squash

@ykakarap ykakarap force-pushed the runtimeruntime-sdk_lifecycle-hooks_before-hooks branch from 6af1ea5 to abb8f5f Compare June 13, 2022 19:31
@fabriziopandini
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2022
@sbueringer
Copy link
Member

Thx!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6b5afa3 into kubernetes-sigs:main Jun 14, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/runtime-sdk Issues or PRs related to Runtime SDK cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants