RSS-Bridge / rss-bridge

The RSS feed for websites missing it
https://rss-bridge.org/bridge01/
The Unlicense
7.37k stars 1.04k forks source link

[prtester] improve prtester.py and prhtmlgenerator.yml for running in forks #4313

Open User123698745 opened 1 month ago

User123698745 commented 1 month ago

This PR will improve prtester.py and prhtmlgenerator.yml for running in forks. It makes the hard-coded repo name and base url to the new rss-bridge-tests repo dynamic/configurable (by using the repo owners name and an optional ARTIFACTS_REPO variable). The Upload tests job will be skipped when the secret RSSTESTER_ACTION is missing (which will always be the case for forks, by default) instead of always failing. Additional small fixes to the Upload tests job: It failed when the prs directory did not exist yet and it failed when there was no change to the html files.

Bockiii commented 1 month ago

Thanks for all the help with this!

Because I've run into this before and maybe you know how to do it:

If you re-run the test manually within github, I've had the behavior that it cant identify the github event number (because the run wasnt caused by a commit but by a manual re-run request) and thus, the github event number defaults to "none".

I'm not sure if you can recreate that and if so, have a fallback to use something else but the github event number.

User123698745 commented 1 month ago

I could not recreate that behavior by re-running the pipeline or single jobs of the pipelines. github.event.number was always set, but to make sure I added a fallback to value none.

User123698745 commented 3 weeks ago

@Bockiii can this get merged?

Bockiii commented 3 weeks ago

I honestly dont understand whats happening in the code and which fail overs are in place.

python prtester.py --artifacts-base-url "https://${{ github.repository_owner }}.github.io/${{ vars.ARTIFACTS_REPO || 'rss-bridge-tests' }}/prs/${{ github.event.number || 'none' }}";

Not every github user has setup a github.io page.

repository: "${{ github.repository_owner }}/${{ vars.ARTIFACTS_REPO || 'rss-bridge-tests' }}"

Definitely no one has setup either this (undocumented) variable or has setup a repo named "rss-bridge-tests".

As an alternative, I would check if the workflow is run in the original repo or not and if not, dont do the upload to a repository. The github action will still have the artifact uploads, so you can just download the html files and check. You could also create an alternate comment in the PR that just says "go to the action and download the artifacts to check" or so.

User123698745 commented 3 weeks ago

I honestly dont understand whats happening in the code and which fail overs are in place.

python prtester.py --artifacts-base-url "https://${{ github.repository_owner }}.github.io/${{ vars.ARTIFACTS_REPO || 'rss-bridge-tests' }}/prs/${{ github.event.number || 'none' }}";

  • Instead of the previous hard-coded url https://rss-bridge.github.io/rss-bridge-tests/prs/{prid} inside the python script itself, the base url gets passed as parameter --artifacts-base-url into it. The base url gets build with dynamic variables inside prhtmlgenerator.yml.
  • github.repository_owner will always return the owner of the repository which the PR targets. For PRs here in the original repo it will result in rss-bridge again. For PRs targeting e.g. my fork it would result to User123698745.
  • vars.ARTIFACTS_REPO is fully optional. It can be used to change the name of the repository, where the files will be uploaded to. If its not set, which will almost always be the case, it will fallback (see the ||) to rss-bridge-tests.
  • github.event.number will always return the PR id, when the Job trigger is pull_request_target or pull_request. prhtmlgenerator.yml does not use any other on trigger, so github.event.number should always be set correctly. Because of your note about re-runs, I added a fallback to none, but I was not able to reproduce that. github.event.number was always set correctly in my test re-runs.
  • So by default for this original repository it will result in to the exact same url https://rss-bridge.github.io/rss-bridge-tests/prs/{prid}, but for forks it will result into a working url for them e.g for my fork https://User123698745.github.io/rss-bridge-tests/prs/{prid}.

Not every github user has setup a github.io page.

repository: "${{ github.repository_owner }}/${{ vars.ARTIFACTS_REPO || 'rss-bridge-tests' }}"

Definitely no one has setup either this (undocumented) variable or has setup a repo named "rss-bridge-tests".

  • Yes, that's why I added this condition if: needs.checks.outputs.WITH_UPLOAD == 'true'. The upload only ever will run when the secret you introduced secrets.RSSTESTER_ACTION is set. By default forks will not have the secret, so by default there will be no upload, but you will still get the comment on all PRs including the helpful warning and error messages and the artifacts on the job run result. Most importantly the job will not fail by default like it currently does for all forks.

As an alternative, I would check if the workflow is run in the original repo or not and if not, don't do the upload to a repository. The github action will still have the artifact uploads, so you can just download the html files and check. You could also create an alternate comment in the PR that just says "go to the action and download the artifacts to check" or so.

  • I am not a fan of limiting that feature to the original only. By default it will behave like you describe (except the alternate comment). Forks can optionally decide by them self if they want to enable uploads by creating their own rss-bridge-tests repository (or using any other name, but then and only then they also need to set vars.ARTIFACTS_REPO to that repository name), setting secrets.RSSTESTER_ACTION to a PAT with access to their rss-bridge-tests repository and enabling github pages on their rss-bridge-tests repository.
User123698745 commented 2 weeks ago

@Bockiii does that explain it?

User123698745 commented 4 days ago

it failed when there was no change to the html files

This now happened for PR https://github.com/RSS-Bridge/rss-bridge/pull/4324 (see the error nothing to commit, working tree clean in the pipeline output https://github.com/RSS-Bridge/rss-bridge/actions/runs/11992047625/job/33431439338?pr=4324), because code was moved, but the prtester.py result stayed the same.

@Bockiii or @dvikan would be nice if you could merge this to get that fixed (or at least only that specific fix git commit -m "$COMMIT_MESSAGE" || exit 0 instead git commit -m "$COMMIT_MESSAGE" )

dvikan commented 4 days ago

i dont wanna mess with python and prtester sorry. @Bockiii