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

Updated to GH Actions V2 #400

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Updated to GH Actions V2 #400

merged 2 commits into from
Jun 10, 2022

Conversation

ddsjoberg
Copy link
Collaborator


Checklist for PR reviewer

  • PR branch has pulled the most recent updates from main branch. Ensure the pull request branch and your local version match and both have the latest updates from the main branch.
  • If a new function was added, function should be included in _pkgdown.yml
  • If a bug was fixed, a unit test was added for the bug check
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features. Review coverage with withr::with_envvar(new = c("NOT_CRAN" = "true"), covr::report()). Before you run, begin a fresh R session without any packages loaded.
  • R CMD Check runs without errors, warnings, and notes
  • usethis::use_spell_check() runs with no spelling errors in documentation
  • Has NEWS.md been updated with the changes from this pull request under the heading indicating the latest version. If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • Has the version number been incremented using usethis::use_version(which = "dev")
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

@ddsjoberg ddsjoberg requested review from SHAESEN2, timtreis and epijim June 9, 2022 20:04
@bailliem bailliem self-requested a review June 10, 2022 09:32
Copy link
Collaborator

@bailliem bailliem left a comment

Choose a reason for hiding this comment

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

Should we remove the action to build the readme on the PR and have this as a manual step and check as part of PR. We had this issue before with building the readme through CI/CD. @epijim and @timtreis

@bailliem bailliem merged commit 585bd78 into develop Jun 10, 2022
@bailliem
Copy link
Collaborator

Approving and disabling the build readme workflow.

@bailliem bailliem deleted the gh_actions_v2 branch June 13, 2022 10:55
@timtreis
Copy link
Collaborator

Just for completeness: Running the README workflow on our PRs before merging is basically our only (not overengineered) way to get up-to-date READMEs without making the main branch no longer write-protected. The README bot just can't push to main because of that. So I don't think it should be generally disabled, just only selectively run on PRs

@ddsjoberg
Copy link
Collaborator Author

I think no one expressed an issue with having the README rendered with every PR. The issue was that when the rendered README.md was pushed to the PR branch, GH was confused and lost the connection to the other GH Actions that were being run. Importantly, this included the R CMD Checks. This had a couple of consequences, 1. GH would block merging because 'required' checks did not pass (even if the checks did pass, but GH didn't know they passed), 2. a few PRs were merged without checking the R CMD Check results.

Adding to the checklist that the README be built was the solution that kept the rendered README update to date, and also allowed us to utilize the GH Actions to run the checks, and block merges from PRs with failing checks.

@timtreis
Copy link
Collaborator

Oh, okay! That sounds like it was a nightmare do find out :D Is this "done done"? Otherwise we could probably trigger the README workflow after everything else had been done

@ddsjoberg
Copy link
Collaborator Author

Oh, okay! That sounds like it was a nightmare do find out :D Is this "done done"? Otherwise we could probably trigger the README workflow after everything else had been done @timtreis

I think @bailliem is making a few minor changes ahead of submitting to CRAN. I am not sure what you mean about triggering the workflow? We added to the checklist to manually build the README, so we won't need the workflow. Also the README build is in the CRAN submission checklist too.

@timtreis
Copy link
Collaborator

Oh okay, missed the manual build part

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.

4 participants