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

Allow staging via krel for kubetest #19488

Closed
wants to merge 1 commit into from
Closed

Allow staging via krel for kubetest #19488

wants to merge 1 commit into from

Conversation

saschagrunert
Copy link
Member

We changed a bunch of dependencies in k/release so we can give this a try again

This adds a new --use-krel flag to kubetest to indicate that we want to stage the sources directly via the API provided by k/release rather than the deprecated push-build.sh script (lives in k/release, too).

The idea is to add another job later on for testing this flag on a non critical Kubernetes build+push job (should live in sig-release).

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 8, 2020
@k8s-ci-robot k8s-ci-robot requested review from fejta and shyamjvs October 8, 2020 10:32
@k8s-ci-robot k8s-ci-robot added area/kubetest sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 8, 2020
@saschagrunert
Copy link
Member Author

cc @kubernetes/release-engineering

xmudrii
xmudrii reviewed Oct 8, 2020
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

Left two nits, but otherwise looks okay to me.

kubetest/main.go Outdated Show resolved Hide resolved
kubetest/main.go Show resolved Hide resolved
@saschagrunert saschagrunert mentioned this pull request Oct 8, 2020
Closed
4 tasks
@saschagrunert saschagrunert force-pushed the stage-via-krel-2 branch 3 times, most recently from 3ce4a44 to 0c01494 Compare October 8, 2020 11:55
@saschagrunert
Copy link
Member Author

Green tests 💚

xmudrii
xmudrii approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 8, 2020
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 12, 2020
@saschagrunert
Copy link
Member Author

Rebased on top of the latest master branch.

@saschagrunert saschagrunert force-pushed the stage-via-krel-2 branch 2 times, most recently from a2315ff to 5068e8b Compare October 14, 2020 08:19
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2020
fejta
fejta reviewed Oct 23, 2020
View reviewed changes
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@saschagrunert saschagrunert mentioned this pull request Oct 27, 2020
Closed
@saschagrunert saschagrunert mentioned this pull request Nov 3, 2020
Closed
@fejta
Copy link
Contributor

fejta commented Dec 1, 2020

/retest

@saschagrunert
Copy link
Member Author

/retest

Hm, it looks like the project does not build any more correctly with the dependency change… 🤔

@spiffxp
Copy link
Member

spiffxp commented Jan 19, 2021

needs rebase

@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 19, 2021
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 20, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/prow Issues or PRs related to prow and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 20, 2021
@saschagrunert
Allow staging via krel for kubetest
2894d43 
This adds a new `--use-krel` flag to kubetest to indicate that we want
to stage the sources directly via the API provided by k/release rather
than the deprecated push-build.sh script (lives in k/release, too).

The idea is to add another job later on for testing this flag on a non
critical Kubernetes build+push job (should live in sig-release).

Signed-off-by: Sascha Grunert <[email protected]>
Signed-off-by: Sascha Grunert <[email protected]>
saschagrunert
saschagrunert commented Jan 20, 2021
View reviewed changes
prow/test/integration/test/overall_test.go
http://www.apache.org/licenses/LICENSE-2.0

http://www.apache.org/licenses/LICENSE-2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like this file is causing the boilerplate test to fail, but I did not change it in this PR. 🤷

@saschagrunert
Copy link
Member Author

needs rebase

Done, please take another look :)

@BenTheElder
Copy link
Member

didn't we land this in kubetest2?
I'm hesitant to keep fiddling with kubetest, the blast radius is huge and the code is a maze.

I'm also concerned about dependency cycles, like #20422

@saschagrunert
Copy link
Member Author

didn't we land this in kubetest2?
I'm hesitant to keep fiddling with kubetest, the blast radius is huge and the code is a maze.

Yes, the main intention of this change is to be able to completely remove the push-build script in a feasible amount of time. Do you think we can switch to kubetest2 in the near future and convert the old jobs to it? If so, then I'm happy to drop this PR.

@fejta
Copy link
Contributor

fejta commented Feb 4, 2021

Can we close or update or lgtm or whatever this PR?

Looks like it also needs a rebase now

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

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

@amwat
Copy link
Contributor

amwat commented Feb 4, 2021

+1 hesistant to make changes to kubetest. We've landed this in kubetest2 successfully and I'm working on a migration plan for kubetest deprecation. I'm inclined to have it solved that way.

@BenTheElder
Copy link
Member

Yes, the main intention of this change is to be able to completely remove the push-build script in a feasible amount of time.

to what end though? is this script actually being actively touched today? if so: why?

As a general rule we avoid substantially changing CI for existing kubernetes release branches, so we'd probably want to wait N releases to no longer have any jobs using this.

Do you think we can switch to kubetest2 in the near future and convert the old jobs to it? If so, then I'm happy to drop this PR.

I'm not sure we should be converting old release branches, regardless of if it's swapping out push-build.sh, or kubetest ...

BenTheElder
BenTheElder reviewed Feb 4, 2021
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

I'm not sure if it's just because the PR is out of date, but this seems prettty heavy on the dependencies for something that should just need to talk to GCS and read some files from disk ...

repos.bzl
name = "com_github_bazelbuild_rules_go",
build_file_generation = "on",
build_file_proto_mode = "disable",
importpath = "github.com/bazelbuild/rules_go",
Copy link
Member

Choose a reason for hiding this comment

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

why are we now importing rules_go as a go library? 🤔

repos.bzl
name = "com_github_cilium_ebpf",
build_file_generation = "on",
build_file_proto_mode = "disable",
importpath = "github.com/cilium/ebpf",
Copy link
Member

Choose a reason for hiding this comment

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

... and ebpf ???

@saschagrunert
Copy link
Member Author

As it looks like that the majority does not want this PR to get merged :)
/close

@k8s-ci-robot
Copy link
Contributor

@saschagrunert: Closed this PR.

In response to this:

As it looks like that the majority does not want this PR to get merged :)
/close

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.

@saschagrunert saschagrunert deleted the stage-via-krel-2 branch February 4, 2021 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fejta fejta fejta left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@justaugustus justaugustus justaugustus requested changes

@hasheddan hasheddan hasheddan approved these changes

@xmudrii xmudrii xmudrii approved these changes

@shyamjvs shyamjvs Awaiting requested review from shyamjvs

Assignees

@justaugustus justaugustus

@fejta fejta

@xmudrii xmudrii

@amwat amwat

@hasheddan hasheddan

Labels
area/kubetest area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@saschagrunert @k8s-ci-robot @fejta @spiffxp @BenTheElder @amwat @justaugustus @xmudrii @hasheddan