[GHA] Remove e2e job dependencies

[kbenzie]

No description provided.

[kbenzie]

Removing the dependencies isn't ideal, I guess the workflow could depend on the cmake workflow?

[pbalcer]

Removing the dependencies isn't ideal, I guess the workflow could depend on the cmake workflow?

They were added to avoid wasting time on E2E runners if UR doesn't even build or CTS doesn't pass.

[kbenzie]

I'll have a look to see what the options are with dependencies then. The workflow dependencies trigger might not have sufficient permissions, though.

[lukaszstolarczuk]

yeeeah, removing these jobs dependencies make sense, cause they won't work here, I guess 😄 but, making dependency to another workflow could be tricky, especially since we want pull_request_target trigger... I think we're circling back to our initial solution with E2E workflows running "in the background"... but it was not entirely secure 😄

[lukaszstolarczuk]

ech.. we could revert your latest change, go back to PR trigger, but skip writting comments and always look into the CI log :/

[lukaszstolarczuk]

or we could add a trigger similar to the current benchmark trigger (on dispatch)

none of these are ideal, I guess

[kbenzie]

ech.. we could revert your latest change, go back to PR trigger, but skip writting comments and always look into the CI log :/

This might make the most sense in the short term while we try and figure out something nicer.

[pbalcer]

No, let's just remove the dependencies if they don't work and reevaluate if we see the jobs waiting too long on runners.

[lukaszstolarczuk]

ech.. we could revert your latest change, go back to PR trigger, but skip writting comments and always look into the CI log :/

This might make the most sense in the short term while we try and figure out something nicer.

tbh, I was trying to find something nicer but ended up in this thread: https://github.com/orgs/community/discussions/15452 and related issue: actions/runner#2347 - waiting for the feature from GHA for quite some time...

[kbenzie]

ech.. we could revert your latest change, go back to PR trigger, but skip writting comments and always look into the CI log :/

This might make the most sense in the short term while we try and figure out something nicer.

tbh, I was trying to find something nicer but ended up in this thread: https://github.com/orgs/community/discussions/15452 and related issue: actions/runner#2347 - waiting for the feature from GHA for quite some time...

Yeah, that's not great UI.
Lets see how this turns out and reevaluate from there.

[lukaszstolarczuk]

I browsed the docs once more and I think, @pbalcer was right in the previous PR - it seems this workflow will be using the base branch's SHA, not the merge SHA: docs and the quote:

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

and a little below

Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event.

so we'll most likely end up with the the status of main instead of current changes...

[kbenzie]

GitHub Actions feel like a big mess. I'll try adding the checkout PR branch step.

For the UI issue, I'm wondering about doing something similiar to || true on the e2e test step with gh pr comment instead of the github/script step. Might workaround the 2 year old UI issue.

[kbenzie]

so we'll most likely end up with the the status of main instead of current changes...

Seems like the checkout action checked out 9e90dec which is actually the HEAD of this PR.

[lukaszstolarczuk]

GitHub Actions feel like a big mess. I'll try adding the checkout PR branch step.

For the UI issue, I'm wondering about doing something similiar to || true on the e2e test step with gh pr comment instead of the github/script step. Might workaround the 2 year old UI issue.

wouldn't gh pr still require some kind of authentication...? if you'd like to use secret for that, it will not work on pull_request trigger, right...?

so we'll most likely end up with the the status of main instead of current changes...

Seems like the checkout action checked out 9e90dec which is actually the HEAD of this PR.

It does seem proper, but can this be caused by you opening PR from an upstream branch, not from a fork...? 😉

[kbenzie]

wouldn't gh pr still require some kind of authentication...? if you'd like to use secret for that, it will not work on pull_request trigger, right...?

It would be using GITHUB_TOKEN to authenticate so would still necessitate pull_request_target trigger to be granted pull-request: write permission. This would only workaround the check showing up as red rather than green.

It does seem proper, but can this be caused by you opening PR from an upstream branch, not from a fork...? 😉

Good point!

[kbenzie]

I'm not sure why the config is broken after adding the steps to checkout the PR commit. I'm quite temtped to revive #1734 in get back to a working state again.

[lukaszstolarczuk]

wouldn't gh pr still require some kind of authentication...? if you'd like to use secret for that, it will not work on pull_request trigger, right...?

It would be using GITHUB_TOKEN to authenticate so would still necessitate pull_request_target trigger to be granted pull-request: write permission. This would only workaround the check showing up as red rather than green.

I don't think the gh pr vs github/script is a solution. We either got red status from any of the steps not working properly (build, tests, anything) or we make the status green (via continue-on-error), but hide the real problem from the reviewers in the log.

[lukaszstolarczuk]

Do we have "approve and run" button for pull_request_target trigger? If not, we can't blindly fetch content from users in here, especially since it runs on our sel-hosted runners

[kbenzie]

Given it shows up in the check UI I would expect the approval and run button also applies but I don't have proof for that.

The more caveats and gotchas turn up the more I'm leaning towards going with #1734 for now.

[lukaszstolarczuk]

Given it shows up in the check UI I would expect the approval and run button also applies but I don't have proof for that.

I just found in other docs that it's not true, unfortunately:

Workflows triggered by pull_request_target events are run in the context of the base branch. Since the base branch is considered trusted, workflows triggered by these events will always run, regardless of approval settings.

[lukaszstolarczuk]

The more caveats and gotchas turn up the more I'm leaning towards going with #1734 for now.

yeah... either this or we come back to idea with dispatch trigger (like benchmarks)

[lukaszstolarczuk]

I'm not sure why the config is broken after adding the steps to checkout the PR commit. I'm quite temtped to revive #1734 in get back to a working state again.

it seems weird, the config looks like it's taken from your commit, not from main...

Perhaps, remove permissions sections from the jobs; perhaps a wrong indentation...?