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.

Sign up for GitHub

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

Jump to bottom

GitHub Flow Migration #330

Closed
wants to merge 2 commits into from
Closed

GitHub Flow Migration #330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ce44a8
GitFlow Migration
ddsjoberg Sep 14, 2023
6aa6e41
rename to match repo/pkg name
ddsjoberg Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Thank you for your Pull Request! We have developed this task checklist from the [Development Process Guide](https://pharmaverse.github.io/admiraldev/devel/articles/development_process.html) to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the `devel` branch until you have checked off each task.
Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the `main` branch until you have checked off each task.

- [ ] Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
- [ ] Code is formatted according to the [tidyverse style guide](https://style.tidyverse.org/). Run `styler::style_file()` to style R and Rmd files
Expand Down
0 admiralext.Rproj → admiraldev.Rproj
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions vignettes/development_process.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ give you the simplest experience of helping to grow and enhance our codebase.
1. To start, you will have either created an issue or commented on an existing
issue to notify that you’d like to contribute code. Then one of the `{admiral}`
core team will assign you to the issue.
1. Create a new feature branch from the development branch `devel` following the naming convention and pull the latest changes - as detailed on the [github usage](git_usage.html#working-with-feature-branches-1) guide.
1. Create a new feature branch from the development branch `main` following the naming convention and pull the latest changes - as detailed on the [github usage](git_usage.html#working-with-feature-branches-1) guide.
1. Familiarize yourself with the `{admiral}` [programming
strategy](programming_strategy.html), and then make the required code updates.
1. Before making a pull request, check the [Pull Request Review Guidance](pr_review_guidance.html) & the following checklist of common things developers miss:
Expand All @@ -36,7 +36,7 @@ strategy](programming_strategy.html), and then make the required code updates.
a. Does your code update have any impact on the vignettes stored in vignettes?
a. Did you update the Changelog `NEWS.md`?
a. Did you build `{admiral}` site `pkgdown::build_site()` and check that all affected examples are displayed correctly and that all new functions occur on the "[Reference](../reference/index.html)" page?
1. Once happy with all the updates, make a [pull request](git_usage.html#pull-request) to merge to the development branch `devel` and link the issue so that it closes after successful merging.
1. Once happy with all the updates, make a [pull request](git_usage.html#pull-request) to merge to the `main` branch and link the issue so that it closes after successful merging.
1. Check that there are no merge conflicts. If there are any, fix them before requesting review. See [solving merge conflicts](git_usage.html#solving-merge-conflicts-in-the-terminal-on-rstudio) guidance.
1. Check the results of the automated `R-CMD check` and `lintr` checks and if any issues consult this [guide](pr_review_guidance.html#common-r-cmd-check-issues).
1. Assign a reviewer from the `{admiral}` core development team – this could be
Expand Down
31 changes: 13 additions & 18 deletions vignettes/git_usage.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,9 @@ This article will give you an overview of how the `{admiral}` project is utilizi

# Branches

- The `main` branch contains the latest **released** version and should not be used for development. You can find the released versions [here](https://GitHub.com/pharmaverse/admiral/releases)
- The `devel` branch contains the latest development version of the package. You will always be branching off and pulling into the `devel` branch. This is set as the default branch on GitHub.
- The `main` branch contains the latest development version of the package.
- The `gh-pages` branches contains the code used to render this website you are looking at right now!
- The `patch` branch is reserved for special hot fixes to address bugs. More info in [Hot Fix Release](release_strategy.html#hot-fix-release)
- The `main`, `devel`, `gh-pages`, `patch` branches are under protection. If you try and push changes to these branches you will get an error unless you are an administrator.
Comment on lines -30 to -31
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep these two items and just remove devel from the second one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added back (but in a new PR). All comments are regarding the new PR


- **Feature** branches are where actual development related to a specific issue happens. Feature branches are merged into `devel` once a pull request is merged. Check out the [Pull Request Review Guidance](pr_review_guidance.html) for more guidance on merging into `devel`.

- **Feature** branches are where actual development related to a specific issue happens. Feature branches are merged into `main` once a pull request is merged. Check out the [Pull Request Review Guidance](pr_review_guidance.html) for more guidance on merging into `main`.

# Working with Feature Branches

Expand All @@ -43,23 +38,23 @@ Each feature branch must be related to an issue. We encourage new developers to

### Naming Branches

The name of the branch must be prefixed with the issue number, followed by a short but meaningful description and the `@<target_branch>` suffix. The latter would be `@devel` in most cases. As an example, given an issue #94 "Program function to derive `LSTALVDT`", the branch name would be `94_derive_var_lstalvdt@devel`.
The name of the branch must be prefixed with the issue number, followed by a short but meaningful description and the `@<target_branch>` suffix. The latter would be `@main` in most cases. As an example, given an issue #94 "Program function to derive `LSTALVDT`", the branch name would be `94_derive_var_lstalvdt@main`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@main does not need to be added as it is the fall back branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed the detail about main as it's not needed


The `@<target_branch>` suffix is used in our CI/CD pipelines, e.g. when running `R CMD check`. It ensures that `{admiral}`'s dependencies such as `{pharmaversesdtm}` and `{admiraldev}` are installed from the specified target branch. So when the target branch is set to `@devel` the dependencies will be installed from those package's respective `devel` branches rather than installing the latest released version. This ensures that we test the development version of `{admiral}` against the development versions of its dependencies. That way all packages are kept in sync.
The `@<target_branch>` suffix is used in our CI/CD pipelines, e.g. when running `R CMD check`. It ensures that `{admiral}`'s dependencies such as `{pharmaversesdtm}` and `{admiraldev}` are installed from the specified target branch. So when the target branch is set to `@main` the dependencies will be installed from those package's respective `main` branches rather than installing the latest released version. This ensures that we test the development version of `{admiral}` against the development versions of its dependencies. That way all packages are kept in sync.

### Create a New Feature Branch from the Terminal (from `devel`)
### Create a New Feature Branch from the Terminal (from `main`)

- Checkout the devel branch: `git checkout devel`
- Checkout the main branch: `git checkout main`
- Pull the latest changes from GitHub: `git pull`
- Create a new branch off the devel branch and switch to it: `git checkout -b <new_branch_name>`
- Create a new branch off the main branch and switch to it: `git checkout -b <new_branch_name>`

### Create a New Feature Branch from GitHub (from `devel`)
### Create a New Feature Branch from GitHub (from `main`)

You can also create a feature branch in GitHub.

- Switch to the `devel` branch
- Switch to the `main` branch
- Type in your new feature branch name
- Click Create branch: `<your_branch_name>@devel` from `devel`
- Click Create branch: `<your_branch_name>@main` from `main`
- Be Sure to Pull down newly created branch into RStudio

```{r, echo = FALSE}
Expand Down Expand Up @@ -113,7 +108,7 @@ We recommend a thorough read through of the articles, [Pull Request Review Guida
Once all changes are committed, push the updated branch to GitHub:
`git push -u origin <branch_name>`

In GitHub, under **Pull requests**, the user will either have a "Compare and pull request" button and/or a "Create Pull Request". The first button will be created for you if GitHub detects recent changes you have made. The branch to merge with must be the `devel` branch (base = `devel`) and the compare branch is the new branch to merge - as shown in the below picture. Please **pay close attention** to the branch you are merging into!
In GitHub, under **Pull requests**, the user will either have a "Compare and pull request" button and/or a "Create Pull Request". The first button will be created for you if GitHub detects recent changes you have made. The branch to merge with must be the `main` branch (base = `main`) and the compare branch is the new branch to merge - as shown in the below picture. Please **pay close attention** to the branch you are merging into!

The issue must be linked to the pull request in the "Development" field of the
Pull Request. In most cases, this will linkage will automatically close the issue and move to the Done column on our project board.
Expand Down Expand Up @@ -153,10 +148,10 @@ knitr::include_graphics("github_done.png", dpi = 144)
Merge conflict is a situation where `git` cannot decide which changes to apply since there were multiple updates in the same part of a file. This typically happens when multiple people update the same part of code. Those conflicts always need to be handled manually (as some further code updates may be required):

```
git checkout devel
git checkout main
git pull
git checkout <feature_branch>
git merge devel
git merge main
```

This provides a list of all files with conflicts In the file with conflicts the conflicting sections are marked with `<<<<<<<`, `=======`, and `>>>>>>>`. The code between these markers must be updated and the markers be removed. Source files need to be updated manually. Generated files like NAMESPACE or the generated documentation files should not be updated manually but recreated after the source files were updated.
Expand Down
10 changes: 5 additions & 5 deletions vignettes/pr_review_guidance.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ knitr::opts_chunk$set(

This document is intended to be guidance for creators and reviewers of pull requests (PRs) in the `{admiral}` package family. PR authors will benefit from shorter review times by closely following the guidance provided here.

A pull request into the `devel` branch signifies that an issue has been "addressed". This issue might be a bug, a feature request or a documentation update. Once a Pull Request is merged into `devel` branch, then the issue(s) can be closed.
A pull request into the `main` branch signifies that an issue has been "addressed". This issue might be a bug, a feature request or a documentation update. Once a Pull Request is merged into `main` branch, then the issue(s) can be closed.

Closely following the below guidance will ensure that our all our "addressed" issues auto-close once we merge `devel`.
Closely following the below guidance will ensure that our all our "addressed" issues auto-close once we merge `main`.

# Review Criteria

For a pull request to be merged into `devel` it needs to pass the automated CI checks that will appear at the bottom of the Pull Request. In addition, the PR creator and reviewer should make sure that
For a pull request to be merged into `main` it needs to pass the automated CI checks that will appear at the bottom of the Pull Request. In addition, the PR creator and reviewer should make sure that

- the [Programming Strategy](programming_strategy.html) and [Development Process](development_process.html) are followed

Expand Down Expand Up @@ -83,15 +83,15 @@ knitr::include_graphics("./pr_review_checkbox.png")

## Complete the Pull Request checklist

The check boxes are linked to the `task-list-completed` workflow. You need to check off each box in acknowledgment that you have done you due diligence in creating a compliant Pull Request. GitHub will refresh the Pull Request and trigger `task-list-completed` workflow that you have completed the task. The PR can not be merged into `devel` until the contributor has checked off each of the check box items.
The check boxes are linked to the `task-list-completed` workflow. You need to check off each box in acknowledgment that you have done you due diligence in creating a compliant Pull Request. GitHub will refresh the Pull Request and trigger `task-list-completed` workflow that you have completed the task. The PR can not be merged into `main` until the contributor has checked off each of the check box items.

```{r echo=FALSE, out.width='120%'}
knitr::include_graphics("./pr_review_actions.png")
```

Please don't hesitate to reach out to the `{admiral}` team on [Slack](https://app.slack.com/client/T028PB489D3/C02M8KN8269) or through the [GitHub Issues](https://github.com/pharmaverse/admiral/issues) tracker if you think this checklist needs to be amended or further clarity is needed on a check box item.

**Note for Reviewers:** We recommend the use of Squash and Merge when merging in a Pull Request. This will create a clean commit history when doing a final merge of `devel` into `main`.
**Note for Reviewers:** We recommend the use of Squash and Merge when merging in a Pull Request. This will create a clean commit history.

# GitHub Actions/CI Workflows

Expand Down
22 changes: 7 additions & 15 deletions vignettes/release_strategy.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,37 +41,29 @@ updates via the community meetings, especially any anticipated breaking changes.

## Quarterly Release

A package release is done in five parts:
A package release is done in four parts:

1) Create a Pull Request from `devel` into the `main` branch. Issues identified in this Pull Request should have work done in separate branches and merged once again into `devel`.
1) Verify that all CI/CD checks are passing for the `devel` into the `main` Pull Request, merge and then bundle up and send off to CRAN. See the [chapter](https://r-pkgs.org/release.html#decide-the-release-type) in R Packages for more details.
1) Verify that all CI/CD checks and then bundle up and send off to CRAN. See the [chapter](https://r-pkgs.org/release.html#decide-the-release-type) in R Packages for more details.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need a PR into main were we update the version number (remove .9000 part and increase version)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that depends on the workflow one uses. the PR checklist from usethis, has you keep your version bump to the release version local until CRAN accepts. But others do different things.

1) If CRAN asks for modifications, repeat steps 1-2 as necessary.
1) Once the package is accepted and available on CRAN, a [GitHub action](https://github.com/pharmaverse/admiral/actions/workflows/pages/pages-build-deployment) is set up to rebuild the `{admiral}` website with all the updates for this release.
1) Use the release button on GitHub to "release" the package onto GitHub. This release onto Github archives the version of code within the `main` branch, attaches the News/Changelog file, bundles the code into a `tar.gz` file and makes a validation report via the GitHub action `validation` from [insightsengineering/validatoR](https://github.com/insightsengineering/thevalidatoR). Please see past [admiral releases](https://github.com/pharmaverse/admiral/releases) for reference and the [Releasing to Github](release_strategy.html#releasing-to-github) section for more details.
1) Any issues fixed in the `main` branches should be merged back into `devel`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a PR to main which adds the .9000 again or should this be done in the first feature branch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we make releases, we should follow the checklist issue created from usethis::use_release_issue(). The last thing to do there is bump the version to xxx.9000. There is no need to wait until the first PR to increase it (although this is certainly something i have done in the past, and either way works perfectly well. But good to stick with standard practices when we can)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use use_release_issue() and follow the list created by the call or should we follow our list? Unfortunately I couldn't find an example of the list created by use_release_issue() in the usethis documentation. Most likely there are many overlaps. I think we should have just one list to follow.

1) Once a release is completed the `main` branch should be merged into `patch` to be ready for hotfixes.

**Quarterly Release:**

* `devel >> main`
* `main >> patch` (To be prepared in case of a needed hotfix)

Be on the look out for an automated check PR whenever `devel` goes into `main` as a future upcoming feature.

## Hot Fix Release

Occasionally we will need to release a hot fix to address a package breaking bug. A hot fix release is done in 6 parts:
Occasionally we will need to release a hot fix to address a package breaking bug. A hot fix release is done in eight parts:

1) Identify all the bugs that need to be fixed for this hot fix release and label with hot fix label.
1) Branches addressing the bugs should have Pull Requests merged into the `patch` branch **NOT** the `devel` branch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add that the patch branch is updated by merging the latest released version into patch.

1) When naming the branch follow the [naming conventions](git_usage.html#implementing-an-issue) guide but use the `@main` suffix
1) Branches addressing the bugs should have Pull Requests merged into a single `patch` branch **NOT** the `main` branch, where the `patch` branch has been created from the most recent release of the package.
1) When naming the branch follow the [naming conventions](git_usage.html#implementing-an-issue) guide but use the `@main` suffix.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the @main suffix must not be used because it refers to the devel version of the upstream dependencies. Instead we must use the latest released version of the upstream dependencies. I am not sure how we can refer to them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

1) Create a Pull Request from `patch` into the `main` branch. Verify that all CI/CD checks are passing, merge and them bundle up and send off to CRAN. See the [chapter](https://r-pkgs.org/release.html#decide-the-release-type) in R Packages for more details.
1) If CRAN asks for modifications, repeat steps 1-4 as necessary.
1) Once the package is accepted and available on CRAN, a [GitHub action](https://github.com/pharmaverse/admiral/actions/workflows/pages/pages-build-deployment) is set up to rebuild the `{admiral}` website with all the updates for this release.
1) Use the release button on GitHub to "release" the package onto GitHub. This release onto Github archives the version of code within the `main` branch, attaches the News/Changelog file, bundles the code into a `tar.gz` file and makes a validation report via the GitHub action `validation` from [insightsengineering/validatoR](https://github.com/insightsengineering/thevalidatoR). Please see past [admiral releases](https://github.com/pharmaverse/admiral/releases) for reference and the [Releasing to Github](release_strategy.html#releasing-to-github) section for more details.
1) These hot fixes should then be merged into the `devel` branch through an additional Pull Request.

**Hot Fix Release**: `patch >> main >> devel`
1) Create a GitHub release on the `patch` branch. Please see past [admiral releases](https://github.com/pharmaverse/admiral/releases) for reference and the [Releasing to Github](release_strategy.html#releasing-to-github) section for more details.
1) These hot fixes should then be merged into the `main` branch through an additional Pull Request.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to add that the development version needs to be added again in main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated


# Releasing to Github
Under the [Releases](https://github.com/pharmaverse/admiral/releases) section in the main repo, select `Draft a New Release`. Proceed by creating the appropriate release tag and header, denoted as `vX.X.X` and `admiral X.X.X` respectively, for the release:
Expand Down
Loading