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

Add 'Check amalgamation' workflow #3693

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Aug 9, 2022

Add a workflow that checks if the sources have been amalgamated and leaves a comment directing to the Contribution Guidelines if they are not.

Because commenting on a PR requires write permission, there are two options for implementing this:

  1. Use 'pull_request_target' and duplicate the code from the ci_test_amalgamation target.
  2. Use two workflows.
    • One workflow using pull_request to run the ci_test_amalgamation target.
    • Another workflow using workflow_run to submit the comment.

The downside to option 2 is that the workflow submitting the comment will run after the main workflows have mostly been completed. This PR implements option 1.

Because of pull_request_target, for security reasons, this workflow won't take effect until merged into the main branch.

I've tested the workflow in my fork.
First-time contributors will see this message: falbrechtskirchinger#3 (comment)
Everyone else: falbrechtskirchinger#3 (comment)

The check_amalgamation.yml workflow should be excluded from the required checks for merging and enabled for first-time contributors.

@falbrechtskirchinger falbrechtskirchinger force-pushed the check-amlgamation branch 6 times, most recently from f61eecc to 36023cd Compare August 9, 2022 19:03
@coveralls
Copy link

coveralls commented Aug 9, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling e122745 on falbrechtskirchinger:check-amlgamation into 8fcdbf2 on nlohmann:develop.

@falbrechtskirchinger
Copy link
Contributor Author

@nlohmann The check_amalgamation.yml workflow should be excluded from the required checks for merging and enabled for first-time contributors.

We still need ci_test_amalgamation, which can be updated in a PR, where this workflow cannot.

@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review August 10, 2022 06:55
@falbrechtskirchinger falbrechtskirchinger force-pushed the check-amlgamation branch 2 times, most recently from 0bb4035 to e122745 Compare August 12, 2022 17:28
@falbrechtskirchinger
Copy link
Contributor Author

@nlohmann After the pending PR touching workflows and cmake/ci.cmake, I'd like to convert some of the code duplicated in Makefiles/CMake/workflows into CMake scripts (i.e., cmake -P).
Meanwhile, this is ready to be reviewed and merged. Additional GitHub setting changes are required to make this work as intended (see #3693 (comment)).

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.11.3 milestone Aug 27, 2022
@nlohmann nlohmann merged commit 4c8cdd7 into nlohmann:develop Aug 27, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the check-amlgamation branch August 27, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants