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

Add issue template with checklist for bumping the minimum required GMT version #1630

Merged
merged 5 commits into from
Dec 4, 2021
Merged

Add issue template with checklist for bumping the minimum required GMT version #1630

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c203987
Add issue template with checklist for bumping the minimum required GM…
Nov 22, 2021
6e4aba9
Format
Nov 22, 2021
2d7924b
Apply suggestions from code review
Nov 29, 2021
dc65172
Add update deprecated syntax in source code
Nov 30, 2021
f5c3348
Merge branch 'main' into template-bump-gmt
Dec 4, 2021
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
29 changes: 29 additions & 0 deletions .github/ISSUE_TEMPLATE/bump_gmt_checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
name: Bump GMT version checklist
about: Checklist for bumping the minimum required GMT version.
title: Bump to GMT X.Y.Z
labels: maintenance
assignees: ''

---

:tada: [GMT X.Y.Z](https://github.com/GenericMappingTools/gmt/releases/tag/X.Y.Z) has been released! It is installable from the
[conda-forge channel](https://anaconda.org/conda-forge/gmt/files) using the following command:

`conda install -c conda-forge gmt=X.Y.Z`

<!-- Please add specific checklist items for the tests, xfail pytest markers, and deprecated syntax that need to be updated. -->

**To-Do**:
- [ ] Bump the minimum required GMT version (1 PR)
- [ ] Update `.github/workflows/cache_data.yaml`
- [ ] Update `.github/workflows/ci_docs.yml`
- [ ] Update `.github/workflows/ci_tests.yaml`
Comment on lines +19 to +21
Copy link
Member

Choose a reason for hiding this comment

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

Probably a little off-topic: The "minimum required GMT version" and "the GMT version used in CI" may be different if we decide to support multiple GMT versions in the future. We probably need to discuss how to test PyGMT with multiple GMT versions when we are close to PyGMT v1.0.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. This could get tricky to manage, so I think it's a good issue topic.

- [ ] Update `doc/install.rst`
- [ ] Update `environment.yml`
- [ ] Update `required_version` in `pygmt/clib/session.py`
- [ ] Update `test_get_default` in `pygmt/tests/test_clib.py`
- [ ] Update compatibility table in `README.rst`
- [ ] Fix failing tests (1 or more PRs)
Copy link
Member

Choose a reason for hiding this comment

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

'Fix failing tests' is listed after 'Bump the minimum GMT version'. Does this imply that we are ok with the CI tests breaking on the main branch for a period of time?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the only way the tests wouldn't fail for a period of time is if all the changes were in a single PR or xfails were added just to be removed shortly after. I would prefer to have the tests break for a short bit in order to split up the tasks and reduce efforts, but of course other opinions are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

Guess it depends on the situation (i.e. how many tests are breaking, and how big of a fix is required). Having failing tests on the main branch means that PRs will need to be merged by those with admin permissions (currently 5, maybe 3 active), so we definitely want to keep that period as short as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I agree that we'll want to make the tests pass again as quickly as possible. We can also adjust the issue checklist from what is in the template on a case-by-case basis.

- [ ] Remove [xfail](https://docs.pytest.org/en/stable/skipping.html#xfail-mark-test-functions-as-expected-to-fail) pytest markers on tests that are now xpass
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be under the "Fix failing tests" header?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should stay separate because these tests do not get listed under the failures, so people will need to pay attention to the full list. For example, here are the failing tests using 6.3 and the three xpasses are not listed:
image

- [ ] Update deprecated syntax in code examples based on the [GMT Changelog](https://docs.generic-mapping-tools.org/latest/changes.html)