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

Automate release, workaround macOS CI errors #179

Merged
merged 3 commits into from May 25, 2023

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented May 15, 2023

Includes

  • Add automatic release workflow as per Automate PyPi release #178
  • Symlink gfortran dylibs to /usr/local/lib on macOS CI so PRMS binaries included in this repo can find them. The current installation in CI via awvwgk/setup-fortran puts libs in /usr/local/opt/gcc/lib/gcc/{version} by default
  • fix pydot usage in pywatershed/analysis/model_graph.py causing CI errors

The workflow currently assumes the repo adopts the same branching scheme as FloPy, where development happens on develop and code is merged to main at release time, while currently the repo merges PRs directly to main

If we adopt the distinction between trunk and development branch this should be good to go and the workflow will be triggered when a branch matching semver format with leading "v" (e.g. v0.1.3) is pushed. If we want to stick to the existing scheme and not introduce a development branch, I can update the workflow to trigger on a different condition, e.g. when commits on main are tagged as release candidates with semver number.

Before merging I can tack on some release docs

@wpbonelli wpbonelli marked this pull request as ready for review May 16, 2023 20:19
@wpbonelli wpbonelli force-pushed the release branch 4 times, most recently from 056e6cc to 8e75235 Compare May 17, 2023 00:09
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #179 (bf763b7) into develop (2b44811) will not change coverage.
The diff coverage is 50.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff            @@
##           develop     #179   +/-   ##
========================================
  Coverage    77.24%   77.24%           
========================================
  Files           50       50           
  Lines         6347     6347           
========================================
  Hits          4903     4903           
  Misses        1444     1444           
Impacted Files Coverage Δ
pywatershed/analysis/model_graph.py 10.43% <0.00%> (ø)
pywatershed/version.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jmccreight
Copy link
Member

Thank you, Wes!
I prefer to only have a main branch which we will snapshot to version branches.

@wpbonelli wpbonelli marked this pull request as draft May 18, 2023 20:59
Copy link
Member

@jmccreight jmccreight left a comment

Choose a reason for hiding this comment

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

When the version number is then merged to main, is there some mechanism that differentiates additional/other commits with that version.txt from the actual version/specific commit? What I've done before is add M.m.p+ into a file like version.txt to avoid that confusion.

Also, the workflow appears to preclude updating all but the most recent release because of this final step that merges to main. IE, if you released 2.1.1, you could not go back and use the workflow to hotfix 1.8.1. Or do I misunderstand that?

@@ -0,0 +1,275 @@
name: Release
Copy link
Member

Choose a reason for hiding this comment

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

@w-bonelli Could you start a README.md in this dir (.github/workflows) with a section describing the release steps/procedure. Can you comment on using git-cliff? I suppose I'll have to squash more often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see this before adding RELEASING.md, sorry. Will move to README.md here

I added git-cliff just to match what flopy does. I could remove it. It works best if commits are conventional, but can be configured to accept any format with conventional_commits = false. With the defaults

conventional_commits = true
filter_unconventional = true

I don't think you'd need to squash since merge commits would be filtered out.

Copy link
Member

Choose a reason for hiding this comment

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

I kinda preferred RELEASING.md, i thought it was your suggestion. We can decide the best place right before merge

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the README.md is good. It being in ci/ seems ok since this isn't something needed by anyone but us since we are the only ones that will make releases.

RELEASING.md Outdated

1. Create a release candidate branch from some preexisting revision on `main`. The branch's name must begin with `v` followed by the semantic version number as described above. Manually updating the version number in `version.py` is not required.
2. Push the branch to the `EC-USGS/pywatershed` repository. This will trigger a job to checkout the release, auto-update version number in `version.py` to match the release branch's name, build and check the package, auto-generate a changelog, upload the package and changelog as artifacts, and create a draft GitHub release.
3. If the distribution passes manual inspection, the draft release can be published via the GitHub UI or CLI. This will trigger a job to tag the snapshot revision with the semantic version number and publish the distribution to PyPI. A final job updates `version.py` to match the just-released major version with incremented minor version number, then opens a draft PR into `main`.
Copy link
Member

Choose a reason for hiding this comment

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

should version.py be version.txt. Will that be added the first time this is run?

Copy link
Member

Choose a reason for hiding this comment

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

or should it be pywatershed/version.py? (ie in the package and not the root?)

Copy link
Contributor Author

@wpbonelli wpbonelli May 19, 2023

Choose a reason for hiding this comment

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

Yes just pywatershed/version.py currently, will fix. We could introduce a version.txt in the root like flopy, in that case ci/scripts/update_version.py would need an update to modify both

Copy link
Member

@jmccreight jmccreight left a comment

Choose a reason for hiding this comment

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

When the version number is then merged to main, is there some mechanism that differentiates additional/other commits with that version.txt from the actual version/specific commit? What I've done before is add M.m.p+ into a file like version.txt to avoid that confusion.

Also, the workflow appears to preclude updating all but the most recent release because of this final step that merges to main. IE, if you released 2.1.1, you could not go back and use the workflow to hotfix 1.8.1. Or do I misunderstand that?

@wpbonelli
Copy link
Contributor Author

The last step assumes most releases are minor (back-compatible features) and drafts a PR setting minor number +=1 patch 0 but it doesn't merge, for hotfixes or major releases the PR could be closed. But maybe it is better to just remove this and keep placeholder values in version files in main like you say and only set them for releases?

@langevin-usgs
Copy link
Contributor

Hey guys, I know I'm late to the party here, but it if there is any way to set this all up to follow the approach that we have for MODFLOW and FloPy, I think that would be best. We have a versioning approach and release strategy with main, develop, and tags. We do not typically go back and make hot fixes to previous releases; instead we are always moving forward. If we want to rethink that for EC in general, then we should, but otherwise we should try and stick with our current recipe. My 2 cents.

@jdhughes-usgs
Copy link
Contributor

Thank you, Wes! I prefer to only have a main branch which we will snapshot to version branches.

I don't think there should be version branches. The commit with the version will be tagged and the release will point to a specific commit. So it will always be possible to get the source for a specific release.

Version branches are also inconsistent with all of our other repositories.

@jmccreight
Copy link
Member

Version branches are required for the workflow. They can then be deleted.

Me, Wes, and Chris just decided to make everything consistent with flopy with one minor exception of the version in version.txt being x.y.z+ on the develop instead of the same or ahead of x.y.z on main.

@jmccreight
Copy link
Member

@jdhughes-usgs i assume you're good with the above. LMK if otherwise

RELEASING.md Outdated Show resolved Hide resolved
ci/scripts/README.md Outdated Show resolved Hide resolved
@jdhughes-usgs
Copy link
Contributor

@jdhughes-usgs i assume you're good with the above. LMK if otherwise

I am ok with it being like flopy, mf6, etc.

The version branches would be deleted since they are not needed after a release is made.

@jdhughes-usgs
Copy link
Contributor

Version branches are required for the workflow. They can then be deleted.

Me, Wes, and Chris just decided to make everything consistent with flopy with one minor exception of the version in version.txt being x.y.z+ on the develop instead of the same or ahead of x.y.z on main.

x.y.z+ on develop is ok. To be most useful would be great if it could be x.y.z+commit_hash on develop. @w-bonelli is there is a way this could be done as part of CI? If we could get this to work we could propagate this to the other repos.

@jdhughes-usgs
Copy link
Contributor

Version branches are required for the workflow. They can then be deleted.
Me, Wes, and Chris just decided to make everything consistent with flopy with one minor exception of the version in version.txt being x.y.z+ on the develop instead of the same or ahead of x.y.z on main.

x.y.z+ on develop is ok. To be most useful would be great if it could be x.y.z+commit_hash on develop. @w-bonelli is there is a way this could be done as part of CI? If we could get this to work we could propagate this to the other repos.

@w-bonelli But we don't have to hold up this PR to implement this, if possible. Maybe this becomes an issue here or in flopy if you think it is possible.

@jmccreight
Copy link
Member

Version branches are required for the workflow. They can then be deleted.
Me, Wes, and Chris just decided to make everything consistent with flopy with one minor exception of the version in version.txt being x.y.z+ on the develop instead of the same or ahead of x.y.z on main.

x.y.z+ on develop is ok. To be most useful would be great if it could be x.y.z+commit_hash on develop. @w-bonelli is there is a way this could be done as part of CI? If we could get this to work we could propagate this to the other repos.

what is the commit hash, that of the current revision? it's hard/impossible to know the commit hash in advance of committing.

@jdhughes-usgs
Copy link
Contributor

Version branches are required for the workflow. They can then be deleted.
Me, Wes, and Chris just decided to make everything consistent with flopy with one minor exception of the version in version.txt being x.y.z+ on the develop instead of the same or ahead of x.y.z on main.

x.y.z+ on develop is ok. To be most useful would be great if it could be x.y.z+commit_hash on develop. @w-bonelli is there is a way this could be done as part of CI? If we could get this to work we could propagate this to the other repos.

what is the commit hash, that of the current revision? it's hard/impossible to know the commit hash in advance of committing.

May not be possible but a google search found this https://github.com/marketplace/actions/commit-hash

@wpbonelli
Copy link
Contributor Author

We have a flopy issue on this topic. Two potential solutions are

I think https://github.com/marketplace/actions/commit-hash can extract version+hash for consumption by other tools (e.g. Docker) but cannot automatically embed a fine-grained version string in the module, which seems like what we want.

@wpbonelli
Copy link
Contributor Author

wpbonelli commented May 23, 2023

I think this is ready. The procedure is described in .github/RELEASE.md. Here is a test run on my fork:

  • Initial run triggered on v0.1.3 branch push
  • Release PR opened from release branch into main. The changelog only shows 1 commit because currently git-cliff is configured to filter out non-conventional commit messages, and omit CI-related commits — see flopy releases for an example of what this will look like in future. edit: the changelog will need to be manually created for the next release, or I can switch the git-cliff config to allow non-conventional commits
  • Run triggered by publishing the release drafted by the initial run. The publish step failed because my fork is not configured for trusted publishing, it should succeed on this repo
  • Reset PR opened into develop

The publish step assumes a pypi deployment environment exists and this repo is configured on PyPI for trusted publishing (I did both). There is no need to store PyPI credentials as repo secrets.

@wpbonelli
Copy link
Contributor Author

wpbonelli commented May 23, 2023

In the test run above, the workflow mistakenly leaves a "+" in version.txt in the released version, and increments the patch version number on the final reset PR, both of which were unwanted leftovers from the flopy approach. I just fixed the former and removed the latter step, so the workflow now follows the procedure as described in .github/RELEASE.md (no more prospective incrementing the patch, just update to the just-released version). Also bumped pywatershed/version.py back down to 0.1.2 since that is the most recent released version.

@wpbonelli wpbonelli marked this pull request as ready for review May 23, 2023 14:14
@wpbonelli wpbonelli force-pushed the release branch 2 times, most recently from bb87a6c to dd2ac7b Compare May 23, 2023 17:09
* add release.yaml workflow to automate release/packaging/publishing
* symlink gfortran dylibs on macOS to workaround CI errors
* reorganize utility scripts under .github/scripts
* add DEVELOPER.md, CONTRIBUTING.md, .github/RELEASE.md
* use git-cliff to make changelogs, introduce cumulative HISTORY.md
* consolidate conda environments in environment.yml in proj root
* add doc optional dependency group to pyproject.toml, use for RTD
* switch provision-with-micromamba to setup-micromamba action
* remove pip requirements files
jmccreight
jmccreight previously approved these changes May 25, 2023
Copy link
Member

@jmccreight jmccreight left a comment

Choose a reason for hiding this comment

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

Thanks alot for cleaning up the envs and restructuring the markdowns.

@jmccreight
Copy link
Member

This PR almost closes #165.

@jmccreight
Copy link
Member

jmccreight commented May 25, 2023

Wes, this is awesome. Thanks for all your work on this.
I did your "what's new" notes for you, in case you were curious to see that.
I'll merge this after all the tests pass again.... rather I'll squash.
closes #178

@jmccreight jmccreight self-requested a review May 25, 2023 04:45
@wpbonelli
Copy link
Contributor Author

@jmccreight sure thing. Thanks, sorry, forgot to do the what's new!

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.

None yet

5 participants