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.

Sign up for GitHub

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

Jump to bottom

Add input option to update pr reports #102

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Add input option to update pr reports #102

merged 4 commits into from
Aug 9, 2024

Conversation

Mrflatt
Copy link
Contributor

@Mrflatt Mrflatt commented Aug 8, 2024 •
edited
Loading

Adds native-image-pr-reports-update option, which replaces old native-image pr report comment with new one. This makes pr's less verbose.

Related to open issue: #101

@oracle-contributor-agreement Oracle Contributor Agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Aug 8, 2024
fniephaus
fniephaus requested changes Aug 8, 2024
View reviewed changes
Copy link
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution! I left a couple of comments and I hope they are easy to address (otherwise let me know). Also, it'd be great if you could sign the OCA so that we can integrate this.

Thanks again,
Fabio

README.md Outdated
@@ -197,6 +197,7 @@ can be replaced with:
| `native-image-musl` | `'false'` | If set to `'true'`, sets up [musl] to build [static binaries][native-image-static] with GraalVM Native Image *(Linux only)*. [Example usage][native-image-musl-build] (be sure to replace `uses: ./` with `uses: graalvm/setup-graalvm@v1`). |
| `native-image-job-reports` *) | `'false'` | If set to `'true'`, post a job summary containing a Native Image build report. |
| `native-image-pr-reports` *) | `'false'` | If set to `'true'`, post a comment containing a Native Image build report on pull requests. Requires `write` permissions for the [`pull-requests` scope][gha-permissions]. |
| `native-image-merge-pr-reports` *) | `'false'` | If set to `'true'`, updates old build report comment with new report on pull requests. Requires `native-image-pr-reports` to be `true` |
Copy link
Member

Choose a reason for hiding this comment

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

Since this is an option for native-image-pr-reports, I'd prefer it uses native-image-pr-reports as the prefix:

Suggested change
| `native-image-merge-pr-reports` *) | `'false'` | If set to `'true'`, updates old build report comment with new report on pull requests. Requires `native-image-pr-reports` to be `true` |
| `native-image-pr-reports-merge` *) | `'false'` | If set to `'true'`, updates old build report comment with new report on pull requests. Requires `native-image-pr-reports` to be `true`. |

action.yml Outdated
@@ -47,6 +47,10 @@ inputs:
required: false
description: 'Post a comment containing a Native Image build report on pull requests.'
default: 'false'
native-image-merge-pr-reports:
required: false
description: 'Merge existing comments with new a Native Image build report on pull requests.'
Copy link
Member

Choose a reason for hiding this comment

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

If I see this right, the option does not really "merge" reports, it just seems to replace/update PR comments with the latest report. Maybe we should call the option something like native-image-pr-reports-update instead?
The description could be adjusted to something like:

Suggested change
description: 'Merge existing comments with new a Native Image build report on pull requests.'
description: 'Instead of posting another comment, update an existing PR comment with the latest Native Image build report.'

src/features/reports.ts Show resolved Hide resolved
src/utils.ts Outdated
@@ -184,6 +184,52 @@ function getGitHubToken(): string {
return core.getInput(c.INPUT_GITHUB_TOKEN)
}

export async function findExistingPrCommentId(
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit, for consistency with other PR-related utils:

Suggested change
export async function findExistingPrCommentId(
export async function findExistingPRCommentId(

@fniephaus fniephaus linked an issue Aug 8, 2024 that may be closed by this pull request
Verbose pr with native-image-pr-reports #101
Closed
@oracle-contributor-agreement Oracle Contributor Agreement
Copy link

Thank you for signing the OCA.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Aug 8, 2024
@Mrflatt Mrflatt force-pushed the feature-merge-pr-reports branch from 83bb6ee to d7919f5 Compare August 8, 2024 10:02
@Mrflatt
Copy link
Contributor Author

Mrflatt commented Aug 8, 2024

Thanks a lot for this contribution! I left a couple of comments and I hope they are easy to address (otherwise let me know). Also, it'd be great if you could sign the OCA so that we can integrate this.

Thanks again, Fabio

Thank you for review! Made changes according to comments

@Mrflatt Mrflatt force-pushed the feature-merge-pr-reports branch from d7919f5 to e5fb903 Compare August 8, 2024 10:04
@Mrflatt Mrflatt changed the title Add input option to merge pr reports Add input option to update pr reports Aug 8, 2024
fniephaus
fniephaus previously approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fniephaus fniephaus self-assigned this Aug 9, 2024
@fniephaus fniephaus force-pushed the feature-merge-pr-reports branch from e7c8c8e to 1a6f131 Compare August 9, 2024 11:47
@fniephaus
Copy link
Member

I've decided to rename the option to native-image-pr-reports-update-existing, which hopefully makes it a bit clearer.

@fniephaus fniephaus merged commit 22cc13f into graalvm:main Aug 9, 2024
93 of 94 checks passed
@Mrflatt Mrflatt deleted the feature-merge-pr-reports branch August 9, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fniephaus fniephaus fniephaus approved these changes

Assignees

@fniephaus fniephaus

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Verbose pr with native-image-pr-reports
2 participants
@Mrflatt @fniephaus