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

Upgrade the build process via GitHub Actions #493

Closed
wants to merge 1 commit into from

Conversation

rpsene
Copy link
Contributor

@rpsene rpsene commented Jun 11, 2024

This commit upgrades the build process by replacing the previous approach with a new one, using a single GitHub Action.

The updated build process is generating the same results and artifacts as the previous one but using a simpler approach, which is easier to maintain.

Screenshot 2024-06-10 at 21 19 44

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

Has something changed to mean that GitHub actions run in a pull request have permission to comment and edit those comments? That’s why the split exists.

@Alasdair
Copy link
Collaborator

I don't think so, if you want actions to comment on a PR you still need two workflows.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

As expected:

00:47:35 +0000 - publish - INFO - This action is running on a pull_request event for a fork repository. Pull request comments and check runs cannot be created, so disabling these features. To fully run the action on fork repository pull requests, see https://github.com/EnricoMi/publish-unit-test-result-action/blob/v2.16.1/README.md#support-fork-repositories-and-dependabot-branches

@jrtc27 jrtc27 closed this Jun 11, 2024
@Alasdair
Copy link
Collaborator

I wonder if there is a better alternative to manually using github-script these days though?

I looked at the error bill reported for the action earlier, and I think it may just be a temporary network failure. Sometimes github actions just stops working due to issues on github's end.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

I don't know what error you're referring to, but if GitHub's network goes kaput it doesn't matter what programming language you're using. And besides, GitHub Actions are written in JavaScript, the github-script step is just a way to write that JavaScript directly rather than making a separate plugin with it that you then call into. AFAIK you get the same API.

@Alasdair
Copy link
Collaborator

Alasdair commented Jun 11, 2024

The two parts of my previous message were not intended to be related in any way, sorry for the confusion.

@Alasdair
Copy link
Collaborator

Alasdair commented Jun 11, 2024

The error that was reported was about publish-tests action failing with a 403 error when fetching the artifact zip file on forks of this repository. After looking some more I think the issue actually occurs when the master branch (on the fork) has not been kept updated - the PR/push action runs on a recent feature branch, then the download/publish results action runs using the master version. If the Node versions don't match between the two it fails.

@rpsene rpsene reopened this Jun 11, 2024
@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

@jrtc27 why you closed this? We can add a step to comment on PRs.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

Because if you're adding another action back you just end up where we already are?

@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

@jrtc27 this is an upgrade to simplify the maintenance of this Actions. We do not need 2 files to handle the simple build process and results publication.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

They have different triggers, so the best fit for GitHub Actions's model is to put each action in its own file. Otherwise you trigger both sets of jobs for each trigger, and have one skipped each time, which is pretty ugly. Unless there's some other alternative you're proposing, I don't see how you can do it in one file.

@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

You can easily filter when to run an action... that's not a big issue.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

If you have a proposal you're welcome to push to the reopened PR. I closed it because what you had was broken by design and I do not see how there's a good alternative to the current approach, which is what the linked documentation above describes as the way to do it, albeit with a different implementation of the separate action (maybe it changed since, or maybe I got it from somewhere else).

@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

I closed it because what you had was broken by design

it was not, I was expecting discussion not you to close it. Anyways, I will check the desired behavior and adjust what is missing...

@Alasdair
Copy link
Collaborator

From my understanding, the issue with combining both actions into a single action is right now, when you create a PR on our github the actions will post a comment with the test results like #491 (comment) which is a nice feature to have.

If you combine the actions into a single action this does not work. The pull request action that runs the tests is run on the external repository where the PR lives before being merged. The action that checks the results and publishes the results runs on this repository and is triggered when it detects a PR workflow finishing, and it has to do so for permission reasons in order to post the comment. I don't know if there is a good way to avoid having two workflows while keeping this feature.

This commit upgrades the build process by
replacing the previous approach with a new
one, using a single GitHub Action.

Signed-off-by: Rafael Sene <[email protected]>
@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

@Alasdair we just need to add

jobs:
build:
if: github.repository == 'sail/sail-riscv'

to ensure it does not run on forks, when a PR is raised.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

Well then you don't get the comment on the PR for the common case of people filing PRs from forks, which is a worse UX, or entire build job even being run?

@rpsene
Copy link
Contributor Author

rpsene commented Jun 11, 2024

You get the comment on the PR that is raised at sail/sail-riscv ...

@jrtc27
Copy link
Collaborator

jrtc27 commented Jun 11, 2024

You get the comment on the PR that is raised at sail/sail-riscv ...

Right, that's the repo the PR is targeting. But that won't magically give it permissions that it didn't before, and that's a feature, because otherwise the PR could change the workflow file to do something else. By having it be the file that's in the repo's own master branch it's under the control of the repo owner, not the PR submitter.

@Timmmm
Copy link
Collaborator

Timmmm commented Jul 25, 2024

Sounds like this is unfortunately impossible... @rpsene any objection to closing this?

@jrtc27
Copy link
Collaborator

jrtc27 commented Aug 9, 2024

Closing; it's been 2 weeks with no response

@jrtc27 jrtc27 closed this Aug 9, 2024
Timmmm pushed a commit to Timmmm/sail-riscv that referenced this pull request Oct 2, 2024
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.

5 participants