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

⚠️ Remove deployment-coordinator [WIP] #2955

Closed
wants to merge 3 commits into from

Conversation

xrstf
Copy link
Contributor

@xrstf xrstf commented May 16, 2023

Summary

This PR is part of a series to work on #2954. As tmc/pkg/ is used in for example cmd/kcp/, it's not a trivial rm -f tmc/ to get rid of the code, but this PR is a much simpler first change.

@openshift-ci openshift-ci bot requested review from qiujian16 and s-urbaniak May 16, 2023 15:15
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 16, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ncdc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 16, 2023

Hi @xrstf. Thanks for your PR.

I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@scheeles
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2023
@davidfestal
Copy link
Member

Wouldn't it make more sense to move the TMC things inside the /tmc repository, so that it can then be globally moved to another GH repo ? Instead of fully removing only part of the TMC code ?

@davidfestal
Copy link
Member

wdyt @sttts ?

@xrstf
Copy link
Contributor Author

xrstf commented May 16, 2023

I was already queueing up to also remove cmd/syncer and related code in another PR, as so far I am not aware of any maintainers for that part of the codebase.

If a standalone repository is a more appropriate resting place, I'm not quite sure how to extract the TMC stuff in a meaningful way, short of just forking kcp-dev/kcp into kcp-dev/kcp-with-tmc ? What would you suggest? :)

If the piece-by-pieceness of this removal is what concerns you, I can also move larger chunks at once. I was concerned with reviewability more than the possibility of a half-complete release soon.

@sttts
Copy link
Member

sttts commented May 16, 2023

Wouldn't it make more sense to move the TMC things inside the /tmc repository, so that it can then be globally moved to another GH repo ? Instead of fully removing only part of the TMC code ?

@davidfestal moving things into a tmc directory and keep everything working is more work I guess. An alternative approach might be to label all PRs removing tmc code. Then one can reconstruct what we removed.

@xrstf
Copy link
Contributor Author

xrstf commented May 16, 2023

We could also just keep a branch name on the commit before the first cleanup PR is merged.

@davidfestal
Copy link
Member

We could also just keep a branch name on the commit before the first cleanup PR is merged.

I like this idea.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2023
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sttts
Copy link
Member

sttts commented May 19, 2023

We could also just keep a branch name on the commit before the first cleanup PR is merged.
I like this idea.

Let's do it.

@sttts
Copy link
Member

sttts commented May 19, 2023

@xrstf xrstf mentioned this pull request May 22, 2023
@xrstf
Copy link
Contributor Author

xrstf commented May 22, 2023

This has been superseded by #2963, which removes more code and has been rebased on the 1.26-rebase already.

@xrstf xrstf closed this May 22, 2023
@xrstf xrstf deleted the remove-deployment-coordinator branch May 22, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants