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

Update bazel image to use multirepo-compatible bootstrap args #2731

Merged
merged 1 commit into from
May 30, 2017

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented May 11, 2017

Additionally, update test-infra jobs to use new bazelbuild image.

Part of #2704.

I think this might work, but I have no idea how to test it. As such, I'm planning to test only on the test-infra repo to start.

/assign @spxtr

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2017
@spxtr
Copy link
Contributor

spxtr commented May 11, 2017

You can run bootstrap locally. Once you push the new image, you can run it on prow manually. Go to a recent job you want to run on https://prow.k8s.io. Click the ⟳ and download the YAML file at the end of the kubectl create call. Modify it with your change, then kubectl create -f it.

@spxtr
Copy link
Contributor

spxtr commented May 11, 2017

(This will obviously be made much easier in the future)

@ixdy
Copy link
Member Author

ixdy commented May 11, 2017

Running locally won't tell me if the PULL_REFS var is being used properly, since I don't know how to construct that.

The prow tip is a good idea.

@@ -25,6 +25,6 @@ bazel clean --expunge
make bazel-test && rc=$? || rc=$?

# Coalesce test results into one file for upload.
"$(dirname "${0}")/../images/pull_kubernetes_bazel/coalesce.py"
"$(dirname "${0}")/../images/bazelbuild/coalesce.py"
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sure there is a reason we don't just put coalesce.py into the bazelbuild image, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason, and it's a pain that your PR will fail tests because it can't find coalesce.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to undo my rename change, but I'm going to stick coalesce.py in the image, which will hopefully make this rename easier in a future change.

@ixdy ixdy force-pushed the bazelbuild-multirepo branch from ce899a6 to c152e84 Compare May 11, 2017 22:40
@ixdy
Copy link
Member Author

ixdy commented May 11, 2017

PTAL.

@spxtr
Copy link
Contributor

spxtr commented May 11, 2017

LGTM, have you run the test on prow?

@ixdy
Copy link
Member Author

ixdy commented May 11, 2017

answer: nope, doesn't work: https://prow.k8s.io/log?pod=ci-test-infra-bazel-54459

Cloning into 'test-infra'...
Traceback (most recent call last):
  File "./test-infra/jenkins/bootstrap.py", line 957, in <module>
    bootstrap(ARGS)
  File "./test-infra/jenkins/bootstrap.py", line 834, in bootstrap
    repos = parse_repos(args)
  File "./test-infra/jenkins/bootstrap.py", line 823, in parse_repos
    raise ValueError('branch:commit must also specify PRs to merge', repos)
ValueError: ('branch:commit must also specify PRs to merge', ['k8s.io/test-infra=master:593074a90647419e6a3bab82c27f2b2246e7d21f'])

@spxtr
Copy link
Contributor

spxtr commented May 12, 2017

Yay, we prevented a minor outage!

@@ -41,6 +41,6 @@ case "${rc}" in
*) echo "Unknown exit code: ${rc}" ;;
esac

./images/pull_kubernetes_bazel/coalesce.py
/coalesce.py
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that an absolute path reference like this is going to make this job harder to test locally (outside of the image). I don't know if that's a non-goal of this particular job, but in general, I thought we tried to have jobs that we could run using just bootstrap.py or even just directly, if you've set up your environment properly.

How do you feel about stripping the leading slash and just making sure that the script is in $PATH to find it? You'll likely have to add / to the PATH in the Dockerfile, but it seems like that would be more forgiving to local development.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair point, though I think

export TEST_TMPDIR="/root/.cache/bazel"

is already going to fail outside of docker.

long-term I think we want to convert this into a scenario, anyway.

@ixdy
Copy link
Member Author

ixdy commented May 23, 2017

It seems like bootstrap.py allows --repo=foo --branch=$PULL_REFS, but it doesn't allow --repo=foo=$PULL_REFS, which I believe should be equivalent. Is there any reason to explicitly disallow this?

@krzyzacy
Copy link
Member

--repo=k8s.io/foo=master:abcd?

@ixdy
Copy link
Member Author

ixdy commented May 23, 2017

yes, I expect that to work. it doesn't right now.

@ixdy
Copy link
Member Author

ixdy commented May 23, 2017

I'm pretty sure there's already a code path to handle checking out a particular commit on a branch not in PR-mode, though. There's even a unit test for this functionality.

Sent #2831 to remove the check. It seems like the unit tests still work as expected.

@ixdy
Copy link
Member Author

ixdy commented May 26, 2017

@k8s-bot test this

@ixdy ixdy force-pushed the bazelbuild-multirepo branch 2 times, most recently from 6286d99 to 23e2214 Compare May 27, 2017 00:06
@ixdy
Copy link
Member Author

ixdy commented May 27, 2017

For fun, I also updated bazel and gcloud in the image.

Things seem to work now, except that bazel 0.5.0 apparently doesn't work on the test-infra repo.

@ixdy
Copy link
Member Author

ixdy commented May 27, 2017

potential fix for bazel 0.5 in pubref/rules_node#21. (We'll need to update our reference, too, of course)

@ixdy ixdy force-pushed the bazelbuild-multirepo branch from 23e2214 to f224d60 Compare May 30, 2017 21:49
Additionally:
* Update bazel to 0.5.0
* Update gcloud to 156.0.0
* Add coalesce.py to the bazelbuild image
* Update test-infra jobs to use new bazelbuild image
@ixdy ixdy force-pushed the bazelbuild-multirepo branch from f224d60 to 90341c7 Compare May 30, 2017 21:51
@ixdy
Copy link
Member Author

ixdy commented May 30, 2017

It all seems to work correctly now. PTAL?

@spxtr
Copy link
Contributor

spxtr commented May 30, 2017

/lgtm
Assuming you ran the test manually.

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

ixdy commented May 30, 2017

yeah, https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/test-infra/2731/pull-test-infra-bazel/3532/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants