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

k-s-testing/frameworks: Add protection & required checks #6363

Merged

Conversation

hoegaarden
Copy link
Contributor

@hoegaarden hoegaarden commented Jan 20, 2018

Based on this discussion I'd like to enable travis as a required status check on kubernetes-sig-testing/frameworks.

I am not sure if I got everything right and if explicitly adding the CLA check is really needed.

@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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 20, 2018
@krzyzacy
Copy link
Member

cc @spiffxp
any reason we use travis rather than prow there?
we just killed off travis in test-infra

@hoegaarden
Copy link
Contributor Author

@krzyzacy There is no particular reason for travis, except I wanted to have a CI setup and that was the fastest way for me to do so.

I have not much idea about prow but would be happy to integrate with that! Is there any docs on how to do so?

Can we maybe merge this now (given my PR makes sense and is correct), so we are protected by CI for now, and then follow up on getting prow set up?

@krzyzacy
Copy link
Member

@hoegaarden prow is a CI system runs on k8s, as long as you can write your test as a k8s pod. Your travis job can be easily rewritten as a prow job.

I don't think branch-protection is the right section, tide should already aware of travis status.
/cc @cjwagner

@cjwagner
Copy link
Member

I don't think branch-protection is the right section, tide should already aware of travis status.

Tide requires all status checks to be green before merging a PR so if there is a travis context on the PR it must be passing to merge serially. However, batch merges (where we test multiple PRs at once) are only tested with Prow jobs, so if you want batches to be tested the job will need to be a Prow job.

The branch protection section controls which status contexts are marked as "Required" on Github. This only affects users who are allowed to click the merge button (which should be a small set of repo maintainers), but it also has the added benefit of requiring the context even if it hasn't been reported which prevents tide from merging a PR before status contexts have been marked with "Job triggered."
If you write the test as a Prow job, the branch protector will automatically mark your job as "Required" without requiring additional configuration.

@spiffxp
Copy link
Member

spiffxp commented Jan 22, 2018

/ok-to-test

I suggested starting with travis first to allow CI to happen w/o going through setting up prow jobs

Is explicitly adding the CLA check required? Asking for followup to a PR I've opened on CLA docs kubernetes/community#1649

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 22, 2018
@cjwagner
Copy link
Member

Is explicitly adding the CLA check required

Tide is configured to only merge PRs with the cncf-cla: yes label so it isn't required for Tide, but it you want to prevent users who have access to the merge button from manually merging a PR with a failing CLA context then it is required.

@hoegaarden
Copy link
Contributor Author

I am happy to look into creating a prow job to test our framework. I guess https://github.com/kubernetes/test-infra#create-a-new-job is the place to start from? I will try to dig around and hopefully add our prow job in antother PR.

Regarding CLA check: I cannot really tell if the CLA check should be explicitely added or not. I added it because I thought it would be the safer option to do so.

@krzyzacy
Copy link
Member

@hoegaarden the pull-community-verify in test-infra will be another good reference, it runs a similar simple make commands for the community repo.

@spiffxp
Copy link
Member

spiffxp commented Jan 23, 2018

/lgtm
I'm going to /lgtm this for now to trial the branch protection working for another org/repo. If this works for setting the CLA check as required I'll update the docs on how to setup the CLA check. "Click here on the github UI" instructions make me antsy

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hoegaarden, spiffxp

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8d74b10 into kubernetes:master Jan 23, 2018
@k8s-ci-robot
Copy link
Contributor

@hoegaarden: I updated Prow config for you!

In response to this:

Based on this discussion I'd like to enable travis as a required status check on kubernetes-sig-testing/frameworks.

I am not sure if I got everything right and if explicitly adding the CLA check is really needed.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants