NOAA-FIMS / FIMS

The repository for development of FIMS
https://noaa-fims.github.io/FIMS/
GNU General Public License v3.0
16 stars 9 forks source link

[Developer Issue]: Use GHA to create a pull request to modify pull requests #660

Open Bai-Li-NOAA opened 4 months ago

Bai-Li-NOAA commented 4 months ago

Description

Comments from @kellijohnson-NOAA: should the documentation GitHub action be run during the PR phase rather than after merging into main. We thought it was not worth implementing at this current point in time but if anyone has ideas for how to ensure that clean up is part of the PR rather than after merging into main that would be amazing ⭐!

Bai-Li-NOAA commented 4 months ago

@iantaylor-NOAA thinks that running doc-and-style on PRs makes a lot of sense. It gives PR authors better feedback on their code styling and gives the reviewers a chance to look over changes to the .Rd files and other documentation updated by doc-and-style. He just implemented that for r4ss (via https://github.com/r4ss/r4ss/commit/5be028c1825610d3c61be85b2691afd1735b75f9) and posted an issue about it here: https://github.com/nmfs-fish-tools/ghactions4r/issues/135. There may be some redundant runs of the workflows but he doesn't think that's a big deal for something that runs pretty fast.

I found an use case in the create-pull-request repository that might work for us.

iantaylor-NOAA commented 4 months ago

My naive implementation in r4ss failed (here) with the following error

Checking the base repository state
  /usr/bin/git symbolic-ref HEAD --short
  fatal: ref HEAD is not a symbolic ref
  /usr/bin/git rev-parse HEAD
  7b34b3d68667987f1623a38a[45](https://github.com/r4ss/r4ss/actions/runs/10116442340/job/27979299598#step:19:49)a96207bff0d4ca
  Working base is commit '7b34b3d68667987f1623a38a45a96207bff0d4ca'
  Error: When the repository is checked out on a commit instead of a branch, the 'base' input must be supplied.

I'm guessing that this was related to the workflow needing to create a pull request to the originating branch rather than main but not having the specifications to do that right. The approach used in the use case that @Bai-Li-NOAA linked above seems more promising.

kellijohnson-NOAA commented 1 month ago

Recently I submitted a PR to {scales} and was intrigued how they use /document and /style as comments in PR to run their automated workflows that way it can be done when wanted rather than forced. What do you think about trying to implement their method as well as ensure that things are styled and documented again before merging into main? Here is a link to their GitHub action.

iantaylor-NOAA commented 1 month ago

@kellijohnson-NOAA, good idea! Additional thoughts after looked through the your scales PR and a few others:

  1. I like that it applies the change directly to the branch rather than opening a separate PR. In r4ss the bug noted in this issue was resolved by just automatically running the documentation and styling in the branch and I appreciate the reduction in notifications but also see how you might want more control.
  2. This, I like that the {scales} approach allows either the original author or anyone else, including a code reviewer to choose when to document things
  3. If nobody triggers the documentation or styling there's a small danger that a future PR would be more confusing to review because of the lag in when the documentation gets applied.
  4. If we use the same codes (I think we definitely should), we would have to remember to use "/document" (which worked in your PR https://github.com/r-lib/scales/pull/460#issuecomment-2427034394) but not "\document" (which was accidentally used by the same developer and didn't run any action in https://github.com/r-lib/scales/pull/414#issuecomment-2426083646)
  5. I can't actually find any use of "/style" in {scales} or elsewhere because the slashes aren't included in the github search and style is too commonly used (or maybe "/style" is just not being used)
  6. Should we just use the same github action, perhaps implemented via usethis::use_github_action() or usethis::use_tidy_github_action() or is there value in integrating this stuff into ghactions4r?
kellijohnson-NOAA commented 1 month ago

@k-doering-NOAA will be back in a few months and I am fine with waiting for her input regarding (6) or @Bai-Li-NOAA can also weigh in given she is the current lead developer of {ghactions4r}.

@iantaylor-NOAA can you elaborate on your trepidations in (3)? I don't understand how something would be more confusing to review if the documentation and styling happened after the review because no one chose to implement /style or /document during the PR?

iantaylor-NOAA commented 1 month ago

Yes, no rush, so happy to defer to @Bai-Li-NOAA on whether to wait for @k-doering-NOAA to return.

Regarding (3), I wasn't sure if these triggers would be the only place that documentation and styler were implemented. If so, then the main branch could get undocumented or unstyled code which and the changes associated with triggers the actions in a future pull request could include both updates within the current PR and the previous one.

However, if we keep the workflow that applies doc and style to main then it's not an issue. If the PR has been cleaned up it won't do anything and if not, it will sort things out.

kellijohnson-NOAA commented 1 month ago

Thanks for the clarification Ian. I am envisioning that we would for sure keep the current workflows but just add the capability to use these additional triggers when wanted.

Bai-Li-NOAA commented 1 month ago

I also agree with Ian's point (3) that relying solely on manual triggers could increase our technical debt around documentation if no one initiates the workflows. In FIMS, we used to automatically run the workflow on each push to the main branch. Now that we've shifted to the dev branch approach, I can update our workflows to trigger some on every push to the dev branch. In addition, use commit-directly: true to apply changes directly to the working branch.

kellijohnson-NOAA commented 1 month ago

Thanks @Bai-Li-NOAA that would be great to change the triggers to run on dev rather than on main with a direct commit. This change will not take effect though until we merge it into main. So we will need to remember to ensure that dev is documented and styled before our next release.