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.

Sign up for GitHub

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

Jump to bottom

🌱 Rename controller context and golang context #2309

Merged
merged 1 commit into from
Sep 5, 2023
Merged

🌱 Rename controller context and golang context #2309

merged 1 commit into from
Sep 5, 2023

Conversation

zhanggbj
Copy link
Contributor

@zhanggbj zhanggbj commented Aug 31, 2023 •
edited
Loading

What this PR does / why we need it:
This is part of the story: refactor capv controller context #2295. Currently working on this incrementally. In this PR, just rename controller context to make it more readable and differentiate from golang context, to be more specific

  • name package sigs.k8s.io/cluster-api-provider-vsphere/pkg/contex as capvcontext
  • remove package alias goctx "context" and just use context
  • in function signature, rename customized context from ctx to xxxxCtx (Leave ctx for golang context as the first parameter, which will be added in future PR )

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 #2295

@k8s-ci-robot k8s-ci-robot requested review from srm09 and yastij August 31, 2023 08:06
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 31, 2023
@zhanggbj zhanggbj marked this pull request as draft August 31, 2023 08:06
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2023
@zhanggbj
Copy link
Contributor Author

This is WIP, but feel free to take a review, especially about the naming, thx!

CC @sbueringer @chrischdi

@zhanggbj zhanggbj force-pushed the refactor_ctx_rename branch from bd576bc to cbd33dc Compare August 31, 2023 08:10
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2023
@zhanggbj zhanggbj force-pushed the refactor_ctx_rename branch 2 times, most recently from aa286a4 to 06dddd9 Compare September 1, 2023 07:41
@zhanggbj zhanggbj marked this pull request as ready for review September 1, 2023 07:46
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2023
@chrischdi
Copy link
Member

/retitle [WIP] 🌱 Rename controller context and golang context

Let's add WIP as long as it is WIP :-)

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Rename controller context and golang context [WIP] 🌱 Rename controller context and golang context Sep 1, 2023
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 1, 2023
@zhanggbj zhanggbj changed the title [WIP] 🌱 Rename controller context and golang context 🌱 Rename controller context and golang context Sep 3, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2023
@zhanggbj
Copy link
Contributor Author

zhanggbj commented Sep 3, 2023

This is ready for review. (will do a rebase later as I assume there will be more conflicts)

sbueringer
sbueringer reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thx. I think this is a huge step in the right direction.

Let's rebase this once #2296 is merged and then get this PR merged ASAP (to avoid further rebases)

controllers/clustermodule_reconciler_test.go Outdated Show resolved Hide resolved
pkg/services/vmoperator/vmopmachine.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@zhanggbj zhanggbj force-pushed the refactor_ctx_rename branch from 06dddd9 to 89ae873 Compare September 5, 2023 03: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 Sep 5, 2023
@zhanggbj zhanggbj force-pushed the refactor_ctx_rename branch from 89ae873 to f9ed29b Compare September 5, 2023 04:39
@zhanggbj zhanggbj force-pushed the refactor_ctx_rename branch from f9ed29b to fda7484 Compare September 5, 2023 06:26
@zhanggbj
Copy link
Contributor Author

zhanggbj commented Sep 5, 2023

Thanks @sbueringer for your review! This is rebased and ready for another look.

@sbueringer
Copy link
Member

sbueringer commented Sep 5, 2023 •
edited
Loading

Thank you very much!!

/lgtm

/assign @chrischdi

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3696e395c099e31b66969e0f0069b69a5513d9a5

@sbueringer
Copy link
Member

/assign @killianmuldoon

@sbueringer
Copy link
Member

Would be good if one of you can also take a look.

Let's try to get this in soon and hold merging other PRs until then

@chrischdi
Copy link
Member

Wow, what a huge effort, thx @zhanggbj !

/approve

/hold if others want to review. OK with unhold though :-)

@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 Sep 5, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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 Sep 5, 2023
@sbueringer
Copy link
Member

Let's go ahead. If there are further reviews & findings we can follow-up.

Just don't want to block others too long

/hold cancel

@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 Sep 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit fb0ccb5 into kubernetes-sigs:main Sep 5, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Sep 5, 2023
@zhanggbj zhanggbj mentioned this pull request Sep 19, 2023
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@srm09 srm09 Awaiting requested review from srm09

@yastij yastij Awaiting requested review from yastij

Assignees

@chrischdi chrischdi

@sbueringer sbueringer

@killianmuldoon killianmuldoon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.
5 participants
@zhanggbj @chrischdi @sbueringer @k8s-ci-robot @killianmuldoon