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 a GitHub Actions workflow for automatic validation of the citation metadata file (CITATION.CFF) #44062

Merged
merged 4 commits into from
Mar 3, 2024

Conversation

abelsiqueira
Copy link
Contributor

Hello!

We noticed that your CITATION.cff had a small issue and fixed it.

In addition to the fix, this Pull Request automates validation of that file using the cffconvert GitHub Action. That way, it's a little bit easier to be robust against future changes to the CITATION.cff file.

BTW it's perfectly fine if you don't feel like accepting this Pull Request for whatever reason -- we just thought it might be helpful is all.

We found your repository using a partially automated workflow; if you have any questions about that, feel free to create an issue over at https://github.com/cffbots/filtering/issues/

On behalf of the cffbots team,
@abelsiqueira / @fdiblen / @jspaaks

Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

  1. The persist-credentials: false option should be passed to actions/checkout@v2
  2. The GITHUB_TOKEN in this workflow should not have any write permissions. This can be accomplished by setting the appropriate top-level permissions key.

@fredrikekre
Copy link
Member

Is it really necessary to have a workflow for a file which is more or less static?

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 7, 2022

Agreed; I would prefer that we not add a workflow solely for this file.

@vtjnash
Copy link
Member

vtjnash commented Feb 7, 2022

The way the workflow is written, it only gets triggered precisely when it is needed, so I think that addition is good

@fredrikekre
Copy link
Member

Still, it will use some resources for every push to check if this file is in the diff, no?

@abelsiqueira
Copy link
Contributor Author

Hi @DilumAluthge, I have updated the workflow following your comment. Let me know if this is correct.

Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

You can further simplify the permissions section. If you specify the permissions you need (e.g. contents: read), and omit all other permissions, the omitted permissions should automatically be set to none, per the GitHub docs here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#permissions

@abelsiqueira
Copy link
Contributor Author

Thanks for the link, @DilumAluthge. I left only contents: read now.

Comment on lines 3 to 15
on:
push:
paths:
- CITATION.cff
Copy link
Member

@DilumAluthge DilumAluthge Feb 10, 2022

Choose a reason for hiding this comment

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

Currently, this workflow will not be triggered on pull requests that come from forks.

I think the behavior we want is this:

  1. If the CITATION.cff file was modified, and this is a pull request, and the base (target) branch of the pull request is either master or release-*, then run the workflow.
  2. If the CITATION.cff file was modified, and this is a push to either master or release-*, then run the workflow.
  3. Else, do not run the workflow.

I think that the following suggestion implements the above behavior, although it would be good for someone else to double-check.

Suggested change
on:
push:
paths:
- CITATION.cff
on:
push:
branches:
- 'master'
- 'release-*'
paths:
- CITATION.cff
pull_request:
branches:
- 'master'
- 'release-*'
paths:
- CITATION.cff

@abelsiqueira
Copy link
Contributor Author

Hi @DilumAluthge, I have updated the workflow and I made some tests:

Let me know what you think.

@DilumAluthge
Copy link
Member

@abelsiqueira Would you mind making a small separate PR that just has the fix to the CITATION.cff file? That change seems pretty uncontroversial and can be merged quickly.

@abelsiqueira abelsiqueira mentioned this pull request Feb 18, 2022
@abelsiqueira
Copy link
Contributor Author

Hi @DilumAluthge, I have created #44236 with just the fix. After that PR is merged I will rebase this one.

@DilumAluthge
Copy link
Member

Now that #44236 has been merged, can you rebase and squash this PR?

@DilumAluthge DilumAluthge added the triage This should be discussed on a triage call label Feb 19, 2022
@DilumAluthge
Copy link
Member

The question for triage is: should we add the GitHub Actions workflow file for validating this file?

@DilumAluthge DilumAluthge changed the title Fix CITATION.cff and add automatic validation of your citation metadata Add a GitHub Actions workflow file for automatic validation of the citation metadata file (CITATION.CFF) Feb 20, 2022
@DilumAluthge DilumAluthge changed the title Add a GitHub Actions workflow file for automatic validation of the citation metadata file (CITATION.CFF) Add a GitHub Actions workflow for automatic validation of the citation metadata file (CITATION.CFF) Feb 20, 2022
Copy link
Member

@DilumAluthge DilumAluthge left a comment

Choose a reason for hiding this comment

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

From a technical point of view, this looks good. Let's see what triage thinks about the idea.

@DilumAluthge DilumAluthge dismissed their stale review February 20, 2022 04:23

The requested changes have been made.

@abelsiqueira
Copy link
Contributor Author

I have rebased and squashed the commits.

@vtjnash
Copy link
Member

vtjnash commented Feb 22, 2022

Can you include a small no-op change to CITATION.cff in this PR, so that we can see this trigger and confirm it is working?

@abelsiqueira
Copy link
Contributor Author

abelsiqueira commented Feb 24, 2022

Can you include a small no-op change to CITATION.cff in this PR, so that we can see this trigger and confirm it is working?

I made a new no-op commit, but I don't expect it to run because the action is not part of the master branch yet. (Edit: https://github.com/JuliaLang/julia/actions/runs/1891858073, does not run because it is not part of the repo yet).

Alternatively, look at this earlier comment: #44062 (comment) specifically the cffbots#2 and cffbots#4 PRs.

@JeffBezanson
Copy link
Member

From triage: this is cool, but does not really seem warranted for an almost-never-changed file with a single citation in it. But I guess we might as well do it, no real cost?

@DilumAluthge DilumAluthge removed the triage This should be discussed on a triage call label Mar 17, 2022
@vtjnash
Copy link
Member

vtjnash commented Jun 11, 2022

I enabled GitHub-provided and citation-file-format/[email protected] actions. Can you push a rebase of this?

CITATION.cff Outdated Show resolved Hide resolved
@abelsiqueira
Copy link
Contributor Author

I have rebased it.

Copy link
Contributor

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

This looks good to me. I see no downsides: the action is only run when really needed. And yeah the file won't be changed often -- but rare events in my experience will be messed up precisely because they are rare and people forget what to watch out for. So I'd be for merging this.

@fingolfin fingolfin added the merge me PR is reviewed. Merge when all tests are passing label Feb 28, 2024
@IanButterworth
Copy link
Member

IanButterworth commented Mar 3, 2024

This branch is quite out of date, so getting CI green isn't worthwhile, but I'm going to merge this without a rebase because:

  1. maintainer edits aren't enabled, so updating isn't trivial, and saves asking the OP to do so
  2. CITATION.cff is not mentioned in the repo before this, so I can't see how it would fail anything

@IanButterworth IanButterworth merged commit e7734ea into JuliaLang:master Mar 3, 2024
6 of 9 checks passed
@IanButterworth IanButterworth removed the merge me PR is reviewed. Merge when all tests are passing label Mar 4, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
…n metadata file (`CITATION.CFF`) (JuliaLang#44062)

Hello!

We noticed that your `CITATION.cff` had a small issue and fixed it.

In addition to the fix, this Pull Request automates validation of that
file using the [cffconvert GitHub
Action](https://github.com/marketplace/actions/cffconvert). That way,
it's a little bit easier to be robust against future changes to the
`CITATION.cff` file.

BTW it's perfectly fine if you don't feel like accepting this Pull
Request for whatever reason -- we just thought it might be helpful is
all.

We found your repository using a partially automated workflow; if you
have any questions about that, feel free to create an issue over at
https://github.com/cffbots/filtering/issues/

On behalf of the cffbots team,
@abelsiqueira / @fdiblen / @jspaaks

---------

Co-authored-by: Max Horn <[email protected]>
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
…n metadata file (`CITATION.CFF`) (JuliaLang#44062)

Hello!

We noticed that your `CITATION.cff` had a small issue and fixed it.

In addition to the fix, this Pull Request automates validation of that
file using the [cffconvert GitHub
Action](https://github.com/marketplace/actions/cffconvert). That way,
it's a little bit easier to be robust against future changes to the
`CITATION.cff` file.

BTW it's perfectly fine if you don't feel like accepting this Pull
Request for whatever reason -- we just thought it might be helpful is
all.

We found your repository using a partially automated workflow; if you
have any questions about that, feel free to create an issue over at
https://github.com/cffbots/filtering/issues/

On behalf of the cffbots team,
@abelsiqueira / @fdiblen / @jspaaks

---------

Co-authored-by: Max Horn <[email protected]>
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.

7 participants