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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

User123698745
Copy link
Contributor

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.

add parameter "--artifact-base-url" and "--artifact-directory"
@Bockiii
Copy link
Contributor

Bockiii commented Oct 22, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@Bockiii can this get merged?

@Bockiii
Copy link
Contributor

Bockiii commented Nov 5, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@Bockiii does that explain it?

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