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

Fix pkg_rpm rules for bazel 0.13+ #63315

Merged
merged 2 commits into from
May 4, 2018
Merged

Fix pkg_rpm rules for bazel 0.13+ #63315

merged 2 commits into from
May 4, 2018

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented Apr 30, 2018

What this PR does / why we need it: next step in addressing #63108; we can use the substitutions supported by bazel 0.13+ to get the pkg_rpm rules to work properly again.

I've also added a filegroup to allow building all of the RPMs easily with bazel build //build/rpms. Note that since these are manual, bazel build //... will still skip building them.

Release note:

NONE

/assign @BenTheElder
cc @sigma

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 30, 2018
@k8s-ci-robot k8s-ci-robot added 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. labels Apr 30, 2018
BenTheElder
BenTheElder approved these changes Apr 30, 2018
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.

/lgtm
/hold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 30, 2018
@sigma
Copy link
Contributor

sigma commented May 1, 2018

can/should the bazel version requirement be encoded somewhere?

@ixdy
Copy link
Member Author

ixdy commented May 1, 2018

@sigma it is, in

kubernetes/build/root/WORKSPACE

Line 42 in d39ff8e

versions.check(minimum_bazel_version = "0.10.0")
.

I intend to bump that as soon as we're using bazel 0.13 on prow.
/retest

@ixdy
Copy link
Member Author

ixdy commented May 1, 2018

alternately we can wait to merge this PR until prow is using a newer bazel.

@BenTheElder have we updated the canary job yet?

@BenTheElder
Copy link
Member

@BenTheElder have we updated the canary job yet?

Nope, that slipped. Doing that now.

@BenTheElder
Copy link
Member

kubernetes/test-infra#7921

@BenTheElder
Copy link
Member

/test pull-kubernetes-bazel-build-canary
/test pull-kubernetes-bazel-test-canary

@ixdy
Copy link
Member Author

ixdy commented May 1, 2018

the bazel build tests aren't likely to be very relevant here, since we no longer build these RPMs in CI.

@BenTheElder
Copy link
Member

BenTheElder commented May 1, 2018 via email

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2018
@ixdy
Copy link
Member Author

ixdy commented May 1, 2018

/test pull-kubernetes-bazel-build-canary
/test pull-kubernetes-bazel-test-canary

1 similar comment
@ixdy
Copy link
Member Author

ixdy commented May 2, 2018

/test pull-kubernetes-bazel-build-canary
/test pull-kubernetes-bazel-test-canary

@ixdy
Copy link
Member Author

ixdy commented May 4, 2018

/retest

@ixdy
Copy link
Member Author

ixdy commented May 4, 2018

/hold cancel
Can I get a re-lgtm please?

@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 May 4, 2018
@dims
Copy link
Member

dims commented May 4, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 4, 2018
BenTheElder
BenTheElder approved these changes May 4, 2018
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.

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dims, ixdy

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-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 01ebccb into kubernetes:master May 4, 2018
@ixdy ixdy deleted the bazel-0.13-rpms branch May 15, 2018 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder approved these changes

@cblecker cblecker Awaiting requested review from cblecker

@zmerlynn zmerlynn Awaiting requested review from zmerlynn

Assignees

@dims dims

@BenTheElder BenTheElder

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. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ixdy @sigma @BenTheElder @dims @k8s-ci-robot @k8s-github-robot