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

git: refactor to use v1 clients adapted to v2 interfaces #15364

Merged
merged 2 commits into from Dec 11, 2019

Conversation

@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Nov 22, 2019

This is a large commit but it's mechanical:

  • update the import from git to git/v2
  • change Clone(name) to ClientFor(org, repo)

Signed-off-by: Steve Kuznetsov skuznets@redhat.com


depends on #15363
/assign @cjwagner @fejta @alvaroaleman

@k8s-ci-robot k8s-ci-robot requested review from Katharine and krzyzacy Nov 22, 2019
@stevekuznetsov stevekuznetsov changed the title git: introduce an interface for repository clients git: refactor to use v1 clients adapted to v2 interfaces Nov 22, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch 3 times, most recently from ed4f8bb to a67a544 Nov 22, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch from a67a544 to c2b640a Nov 25, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch 4 times, most recently from fd7ce56 to 7a90248 Nov 25, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch 2 times, most recently from d54ba92 to 28d3bb8 Dec 3, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch from 28d3bb8 to 7485216 Dec 3, 2019
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch from 40b9052 to dd58617 Dec 10, 2019
@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 10, 2019

We just continue to use the original v1 implementation but now behind the v2 interface.

Let's change one binary to use the v2 interface. Once we prove that works, update the rest of the components.

@alvaroaleman

This comment has been minimized.

Copy link
Member

alvaroaleman commented Dec 10, 2019

Let's change one binary to use the v2 interface. Once we prove that works, update the rest of the components.

Do you think that makes a difference? Because if something is broken we will rollback the change anyways and at that point it probably doesn't matter if the change broke everything or just one component.

It would be a different story if we had components where we don't care if they are broken temporarily but I don't think we have anything like that for git

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Dec 10, 2019

Erick I still think you fundamentally misunderstand this pull request. What are you saying?

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Dec 10, 2019

The only change here is that the callsite goes to an interface, wrapping the original, unchanged v1 clients..

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch 2 times, most recently from db7f461 to 0ea56f5 Dec 10, 2019
@stevekuznetsov

This comment has been minimized.

Copy link
Contributor Author

stevekuznetsov commented Dec 10, 2019

Also I would love to not have to rebase this a fifth time ...

@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch from 0ea56f5 to 5c10a48 Dec 10, 2019
@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 10, 2019

Other reviewers: PLEASE DO NOT APPROVE THIS PR UNTIL IT LIMITS THE NUMBER OF CHANGED BINARIES TO ONE.

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 10, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 10, 2019
@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Dec 10, 2019

Other reviewers: PLEASE DO NOT APPROVE THIS PR UNTIL IT LIMITS THE NUMBER OF CHANGED BINARIES TO ONE.

@fejta This feels a little hostile. Can you explain this in the context of the responses from Alvaro and Steve so we can make a decision communally?

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 10, 2019

I'm explaining why I want it a release with this change in just one binary. Steve disagrees and says fine I find someone else to approve.

@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 10, 2019

Both for this PR and more importantly in general, I would like us to canary large changes in a single binary so we have some evidence they work correctly before refactoring the test of the code base.

It is important to design the blast radius of these types of changes. Mistakes will happen and so making the impact of the mistake as small as possible is equally important as ensuring the mistake is as infrequent as possible.

Cole and Steve, can you figure out the plan for this PR? I'm happy for you two to unhold and move this PR forward once you two have a satisfactory plan.

…pick-rollout"

This reverts commit c42ce36, reversing
changes made to 9f74236.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
This is a large commit but it's mechanical:

 - update the import from git to git/v2
 - change Clone(name) to ClientFor(org, repo)

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov stevekuznetsov force-pushed the stevekuznetsov:skuznets/git-v2-step-5 branch from 5c10a48 to ee1f9ba Dec 11, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 11, 2019
@fejta

This comment has been minimized.

Copy link
Contributor

fejta commented Dec 11, 2019

/hold cancel

@fejta
fejta approved these changes Dec 11, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 11, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, fejta, stevekuznetsov

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 merged commit 29a8ab2 into kubernetes:master Dec 11, 2019
4 of 5 checks passed
4 of 5 checks passed
tide Not mergeable.
Details
cla/linuxfoundation stevekuznetsov authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-yamllint Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.