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 CI #11

Merged
merged 1 commit into from
May 3, 2024
Merged

Add CI #11

merged 1 commit into from
May 3, 2024

Conversation

zsusswein
Copy link
Collaborator

@zsusswein zsusswein commented May 1, 2024

Adds an initial Github Actions setup. Mainly based on
usethis::use_tidy_github_actions() with some changes:

  • The R CMD CHECK runs are pared back to only Ubuntu/OSX/Windows latest
    versions
  • The codecov check is dropped because it had some setup with a token
    required. Not totally sure what the deal is there, so I replaced it
    with a simple unit test run for now.
    • EDIT: Got it working. Left the unit tests as a post-push hook.
  • I added a check that NEWS.md is modified with each PR into main, so we
    remember to update it with the changes
  • I added in a check that fixup commits are blocked, so we remember to
    autosquash before merging
  • Set up the pkgdown deployment

I also added in a placeholder R script and test file to run on the
actions. These will be replaced with real functionality in future PRs.

@zsusswein zsusswein force-pushed the 06-setup-ci branch 2 times, most recently from d5d1200 to 7d48a88 Compare May 1, 2024 21:41
@zsusswein zsusswein changed the title Add tidy CI Add CI May 1, 2024
@zsusswein zsusswein marked this pull request as ready for review May 1, 2024 21:44
@zsusswein
Copy link
Collaborator Author

zsusswein commented May 1, 2024

I've set all but R CMD check to be required. I figured we might see an occasional weird Windows thing or a NOTE on the build that's not worth immediately addressing. We could require everything or require nothing; I don't feel especially strongly.

@zsusswein zsusswein linked an issue May 1, 2024 that may be closed by this pull request
Closed
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

This overall looks good to me.

I think I would definitely make Rmd checks as required (usually these are the only thing that are required and also sometimes the only CI people have). This is because they are testing the integrity of your package. NOTEs etc shouldn't cause a build failure. I can see a world where things like the NEWS update might not be wanted (like a hotfix) and so perhaps that doesn't need to be required.

I also see some issues with DESCRIPTION groups that don't exist. Finally you likely want to add some badges for these to your README.

The codecov check is dropped because it had some setup with a token required. Not totally sure what the deal is there, so I replaced it with a simple unit test run for now.

This is an odd one as it should work as long as someone has authorised cdcgov to use codecov. They must have done so as we are using it in the EpiAware subdirectory. I agree we can move on but lets make an issue with some detail and aim to get it resolved asap.

.github/workflows/pr-commands.yaml Show resolved Hide resolved
@seabbs
Copy link
Collaborator

seabbs commented May 2, 2024

Whilst we are here we might also want to enable a merge queue so that we have to worry less about rebasing code al the time (it gets rebased and then all CI run in the merge queue and then merged if it passes and bounced if it doesn't)

@zsusswein
Copy link
Collaborator Author

I think I would definitely make Rmd checks as required

Ha, ok! Switched it around so it's the R CMD checks that are required.

also see some issues with DESCRIPTION groups that don't exist

Fixed.

Finally you likely want to add some badges for these to your README.

I think this is a nice-to-have, but would like to leave out of scope for this PR. Moved to #12.

I agree we can move on but lets make an issue with some detail and aim to get it resolved asap.

Took a little messing around (sorry for all the notifications!) but it's working now.

we might also want to enable a merge queue

I don't mind manually rebasing right now. Let's revisit if this picks up steam?

Copy link

codecov bot commented May 3, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

All looking pretty good. A few last questions/comments.

As per CFA policy I think you also want pre-commit to be required and also probably your linting check? If you wanted to be very strict with development you could also add code coverage to that

@zsusswein
Copy link
Collaborator Author

As per CFA policy I think you also want pre-commit to be required

probably your linting check

Linting is in pre-commit so that should be covered.

Adds an initial Github Actions setup. Mainly based on
`usethis::use_tidy_github_actions()` with some changes:

- The R CMD CHECK runs are pared back to only Ubuntu/OSX/Windows latest
  versions
- The codecov check is dropped because it had some setup with a token
  required. Not totally sure what the deal is there, so I replaced it
  with a simple unit test run for now.
- I added a check that NEWS.md is modified with each PR into main, so we
  remember to update it with the changes
- I added in a check that fixup commits are blocked, so we remember to
  autosquash before merging

I also added in a placeholder R script and test file to run on the
actions. These will be replaced with real functionality in future PRs.
Copy link
Collaborator

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM. Oh my is this collapsing of commits not via a squash merge CFA policy? I find it very hard to review final PR changes and I don't really get the reason for it vs a squash merge either.

@seabbs seabbs merged commit ff136bf into main May 3, 2024
10 checks passed
@seabbs seabbs deleted the 06-setup-ci branch May 3, 2024 16:39
@zsusswein
Copy link
Collaborator Author

No, I was just rebasing. I can stop if you prefer a squash at the end.

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.

CI
2 participants