Skip to content

[CI] Don't restrict pre-commit for PRs from inside the repo #10442

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

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

aelovikov-intel
Copy link
Contributor

If the PR's source branch is already inside origin then it is known to be safe.

If the PR's source branch is already inside `origin` then it is known to
be safe.
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner July 17, 2023 23:24
@aelovikov-intel
Copy link
Contributor Author

@aelovikov-intel aelovikov-intel requested a review from bader July 17, 2023 23:25
@bader
Copy link
Contributor

bader commented Jul 18, 2023

Tested in https://github.com/intel/llvm/pull/10441/checks.

You use sycl-devops-pr prefix for your test branch and we already allow running modified scripts for them. Right?

@aelovikov-intel
Copy link
Contributor Author

Tested in https://github.com/intel/llvm/pull/10441/checks.

You use sycl-devops-pr prefix for your test branch and we already allow running modified scripts for them. Right?

Yes, it's been described in PR/commit message for #10002.

@bader
Copy link
Contributor

bader commented Jul 18, 2023

Tested in https://github.com/intel/llvm/pull/10441/checks.

You use sycl-devops-pr prefix for your test branch and we already allow running modified scripts for them. Right?

Yes, it's been described in PR/commit message for #10002.

Right. I don't think https://github.com/intel/llvm/pull/10441/checks really checks this change.

@aelovikov-intel
Copy link
Contributor Author

Right. I don't think https://github.com/intel/llvm/pull/10441/checks really checks this change.

I think it does. Initially I've created the PR from a branch with the same name in my fork and its testing didn't start - see https://github.com/intel/llvm/pull/10440/checks.

@bader
Copy link
Contributor

bader commented Jul 18, 2023

Right. I don't think https://github.com/intel/llvm/pull/10441/checks really checks this change.

I think it does. Initially I've created the PR from a branch with the same name in my fork and its testing didn't start - see https://github.com/intel/llvm/pull/10440/checks.

But the goal of the change is to allow pre-commit for branches in intel/llvm, not for private forks. Isn't the goal of this change to enable pre-commit testing for LLVM pulldown PR? If so, we need to test it. IMO, negative testing is not enough and the use case from https://github.com/intel/llvm/pull/10441/checks should be already working.

@aelovikov-intel aelovikov-intel merged commit 0697e68 into sycl Jul 18, 2023
@aelovikov-intel aelovikov-intel deleted the sycl-devops-pr/allow-pulldown branch July 18, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants