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

Drop boskos import and client-go@v11 #20422

Closed

Conversation

howardjohn
Copy link
Contributor

@howardjohn howardjohn commented Jan 8, 2021

This contains a replace for tektoncd, will not be needed when
tektoncd/pipeline#3668 is merged. I am opening
the PR now for early feedback.

This is an attempt at fixing
#20421. This is done by
making kubetest a submodule. Alternatives are to remove it entirely (if
its fully replaced with kubetest2) or move it to its own repo

If we don't want to make kubetest a submodule, its also possible to merge this, update sigs.k8s.io/boskos to the latest k8s.io/test-infra, then send a new PR to make kubetest not a submodule.

This contains a `replace` for tektoncd, will not be needed when
tektoncd/pipeline#3668 is merged. I am opening
the PR now for early feedback.

This is an attempt at fixing
kubernetes#20421. This is done by
making kubetest a submodule. Alternatives are to remove it entirely (if
its fully replaced with kubetest2) or move it to its own repo.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 8, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @howardjohn. Thanks for your PR.

I'm waiting for a kubernetes 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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/config Issues or PRs related to code in /config area/kubetest area/prow/bump Updates to the k8s prow cluster sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: howardjohn
To complete the pull request process, please assign chaodaig after the PR has been reviewed.
You can assign the PR to them by writing /assign @chaodaig in a comment when ready.

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

@alvaroaleman
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot 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 Jan 8, 2021
@k8s-ci-robot
Copy link
Contributor

@howardjohn: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-test-infra-bazel c1b91a9 link /test pull-test-infra-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

"sigs.k8s.io/boskos/common"
)

func TestConfig(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I just realized what this test was actually doing - you are right, looks like we shouldn't just remove it.

BTW, an alternative is to remove it now, then once boskos updates we can add it back. It just depends if we want to stop importing boskos forever, or just to avoid the bad client-go import temporarily. Probably long term its good to not have a circular dependency.

@chases2 chases2 removed their request for review January 12, 2021 01:53
@BenTheElder
Copy link
Member

kubetest2 won't be able to replace kubetest in all jobs for some time (it's not a super high priority to eliminate, but kubetest2 has been much easier to extend to new tasks), but being a distinct go module SGTM. The rest of test-infra should not be importing kubetest and vice versa.

@BenTheElder
Copy link
Member

BenTheElder commented Jan 15, 2021

I don't think this trivially maps to bazel though, because bazel has a single set of dependencies and includes the go dependencies.

I'm not sure we need to be able to build kubetest with bazel given it's basically frozen except for bug-fixes (EDIT: and I think the docker image we ship it in has used go build), but only if there's a new presubmit covering changes to kubetest/ that ensures it still builds etc since right now we just bazel build //... + bazel test //... the whole repo.

Alternatively if someone has a strong opinion about kuebtest and bazel, we could maybe do a sub-WORKSPACE.

@BenTheElder
Copy link
Member

If we don't want to make kubetest a submodule, its also possible to merge this, update sigs.k8s.io/boskos to the latest k8s.io/test-infra, then send a new PR to make kubetest not a submodule.

We can't just update boskos as-is? Do we have in import cycle? :yikes:

Can we not import test-infra in boskos?

@howardjohn
Copy link
Contributor Author

@BenTheElder yes there is a cycle. Info in #20421. Its probably not that hard to not import test-infra in boskos but I am not familiar with it much - the functions here don't seem to major: #20421 (comment)

@alvaroaleman
Copy link
Member

Can we not import test-infra in boskos?

We can, but that would be a bit annoying, as it currently imports a set of utility code from test-infra. I would rather make test-infra not depend on boskos, by replacing the current golang-based config test with the boskos checkconfig binary (which has the sideeffect that we test the config against the same boskos version we actually use, rather than some boskos version from the last time someone bothered to bump the dependency) and make kubetest its own submodule. WDYT?

@k8s-ci-robot
Copy link
Contributor

@howardjohn: 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2021
@BenTheElder
Copy link
Member

BenTheElder commented Jan 16, 2021

and make kubetest its own submodule. WDYT?

Have to solve the part where presubmit build + test uses bazel which does not have submodules.
EDIT: this is doable, but it requires adding a non-bazel presubmit and removing the submodule from the bazel build, which may not be agreeable to everyone.

I don't think test-infra is a great place to store utility packages imported by other repos, itself being a mono-repo, and it's not surprising that test-infra tools would want to interact with boskos.

@BenTheElder
Copy link
Member

BenTheElder commented Mar 2, 2021

I think maybe we can switch kubetest back to a simple local boskos client instead of pulling in the real client. I don't think multi-go-module is going to work as expected with bazel.

edit: discussing today in #prow kubernetes slack

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 31, 2021
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 30, 2021
@BenTheElder
Copy link
Member

I think maybe we can switch kubetest back to a simple local boskos client instead of pulling in the real client. I don't think multi-go-module is going to work as expected with bazel.

I think this is the best path forward, if anyone wants to take this up. Because the boskos libraries are similarly part of the Kubernetes project and under the same ownership, license, etc., we could even more or less just copy in the bits kubetest needs.

It also might be a good thought, on the other hand, to update the boskos project to not publish a client that requires transitively depending on Kubernetes, however I don't know that anyone is actively maintaining there just this moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config Issues or PRs related to code in /config area/kubetest area/prow/bump Updates to the k8s prow cluster cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants