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

Spec PR review action - POC #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lukasj
Copy link
Contributor

@lukasj lukasj commented Sep 9, 2021

Defines a workflow for verification of specification PRs.

The workflow is currently set to react on PRs to master/main branch IF the PR description contains Specification PR template. It also runs after every modification of such PR.

The "custom" nodejs based action included here takes a list of modified files within the PR and PR description as an input, goes through the 1st group of checks defined by the spec_review_checklist.md and outputs findings as MD formatted text

The workflow then adds this output as a comment to the PR (the comment is either created or updated after each modification of the PR/action run)

Signed-off-by: Lukas Jungmann <lukas.jungmann@oracle.com>
@netlify
Copy link

netlify bot commented Sep 9, 2021

✔️ Deploy Preview for jakartaee-specifications ready!

🔨 Explore the source changes: e2bc26c

🔍 Inspect the deploy log: https://app.netlify.com/sites/jakartaee-specifications/deploys/613a0c0e349337000890d8ad

😎 Browse the preview: https://deploy-preview-421--jakartaee-specifications.netlify.app

@starksm64 starksm64 self-requested a review September 10, 2021 02:04
Copy link
Contributor

@starksm64 starksm64 left a comment

Choose a reason for hiding this comment

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

I would suggest we avoid checking in the large number of node_modules files by following the suggestion in the following doc to produce a single index.js file using @vercel/ncc:
https://docs.github.com/en/actions/creating-actions/creating-a-javascript-action

@lukasj
Copy link
Contributor Author

lukasj commented Sep 10, 2021

Whatever you think is the best, I only need to know where to (re-)create this PR... One drawback I can see is in syncing changes between those two repos. On the benefits side, "test" won't spam this repo and it may be easier to distinguish between dev and stable versions. Compiled version should also run faster - but I do not think speed matters that much in this particular case.

...and forgot to mention that for current testing, one can use my repo: https://github.com/lukasj/repo

@starksm64
Copy link
Contributor

starksm64 commented Oct 6, 2021

So after I had a base rewritten in typescript I started trying to test it in my fork repo, I ran into problems due to the issues described here:
https://gitpro.ttaallkk.topmunity/t/github-actions-are-severely-limited-on-prs/18179/8

Since creating PRs from forks is how we do reviews, it seems github actions are unusable, however I'm not clear on if the problem is because I'm testing in the fork repo. Maybe we need to merge a simple workflow to test if an action is able to add/update comments on the specifications repo.

Now I'm just looking at creating an action using the following rest api java binding:
https://github.com/hub4j/github-api.git

This can be run externally, or via docker I suppose.

@starksm64
Copy link
Contributor

Here is an example of doing the same work to check a PR and update a comment as the action was doing using the org.kohsuke:github-api GitHub REST binding for Java. Of course this needs to be run manually rather than from a workflow event.
https://github.com/jakartaredhat/githubactions/blob/main/src/test/java/jakarta/PRTests.java

The work to update @lukasj POC into TypeScript is available in the actions branch of this repo:
https://github.com/jakartaredhat/specifications/tree/actions

It only has the first check regarding conformance to the PR template ported since I could not get past the permission errors to create a comment from this forked repo.

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