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

[core] Add pr-html-generator #2525

Merged
merged 24 commits into from
Mar 25, 2022
Merged

Conversation

Bockiii
Copy link
Contributor

@Bockiii Bockiii commented Mar 24, 2022

Greetings,

this is a new quality checking workflow.

This will automatically generate the html output of any changed bridges of a PR, for any context of the bridge.

You can see it in action here (its a dummy repo of mine, hardwired to a specific PR. the code in this PR uses the actual PR number that it's running for).

@em92 this would replace the manual process/script for testing. This will also check/indicate if all example/default values are correctly defined. It tests all contexts so it's basically an automatic full-test of this specific bridge. Thoughts?

Additionally: I wrote this in python but it's probably just as doable in php. If you want to keep even the helper/quality scripts php only, I would still advocate for merging and then someone else can do the php version :)

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 24, 2022

The check fails for this bridge since I didn't change any bridges btw. That doesnt happen that often on normal PRs and even if, it would be hard to tell which bridge to test if someone changes something in a core file :)

@yamanq
Copy link
Contributor

yamanq commented Mar 24, 2022

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 24, 2022

@yamanq ouuuuuh, thats exactly what I was looking for for 20 minutes :D Thanks, I'll see what I can do since its a github action thats doing it.

files: |
./results/*.html
- name: Upload results to PR
uses: gavv/pull-request-artifacts@v1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how actions work exactly, but I forked the one used in this PR, changed a line, and tagged it. Would this work then?

Suggested change
uses: gavv/pull-request-artifacts@v1.0.0
uses: yamanq/pull-request-artifacts@v1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you changed it in the wrong line. Try to build the string in line 156

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed and tested this time. See test and required changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, thanks! I will adap the action. I currently have another issue (as you can see from the actions) that there seems to be a permission issue somewhere thats not happening in my own branch.

@yamanq
Copy link
Contributor

yamanq commented Mar 25, 2022

Here is the issue and possible workarounds.

Edit: This might be the easiest workaround to implement.

@yamanq
Copy link
Contributor

yamanq commented Mar 25, 2022

Looking at the existing workflow, it already uses pull_request_target, so ignore my previous comment. This article might be more helpful.

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 25, 2022

Yeah, I've been trying furiously as you can see :D I've also seen that article, but I haven't tried the double-set yet (basically reacting on lint workflows for example).

Either I dont understand how pull_request_target should works or it's not working as intended (it has not fired once in this whole PR).

@yamanq
Copy link
Contributor

yamanq commented Mar 25, 2022

Is it possible that the action must exist in the target branch first?

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 25, 2022

I dont think so. The same action (just with on: pull_request) works fine. You can see the runs and failed runs of it in the repo actions history. It's running... just not with pull_request_target... and I have no idea why...

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 25, 2022

Oh wait... there could be something to it...

Let me try with my other repo

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 25, 2022

Okay.... that was actually it. And in my shame, it actually makes sense. You can add a workflow.yml in a PR and it will run it no problem (if you use on: pull_request). It will NOT do it if you use pull_request_target because you could just go "echo secrets.*" or something like that in a new PR on any repo

So.. the solution is: accept that the workflow works in my repo and then just merge ^^

@Bockiii
Copy link
Contributor Author

Bockiii commented Mar 25, 2022

Okay, I will merge this commit then. Worst thing that can happen is that a few tests fail for new PRs. Changes can be done in further PRs.

Thanks a whole lot @yamanq !

@Bockiii Bockiii merged commit aff442d into RSS-Bridge:master Mar 25, 2022
@Bockiii Bockiii mentioned this pull request Mar 26, 2022
Kwbmm pushed a commit to Kwbmm/rss-bridge that referenced this pull request Jun 17, 2022
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