Skip to content

Add slash command '/test-gmt-dev' to test GMT dev version#831

Merged
seisman merged 7 commits intomasterfrom
slash-test-gmt
Feb 2, 2021
Merged

Add slash command '/test-gmt-dev' to test GMT dev version#831
seisman merged 7 commits intomasterfrom
slash-test-gmt

Conversation

@seisman
Copy link
Member

@seisman seisman commented Feb 1, 2021 •
edited
Loading

Description of proposed changes

This PR adds the slash command /test-gmt-dev which can trigger the GMT Dev Tests workflow. So we can test GMT master branch anytime, without requesting reviews from other maintainers or teams (Also fixes #828).

As you may know/remember, the slash command can only read the configurations in the master branch, so we can't test this PR unless we merge it into the master branch.

To test this PR, I forked the PyGMT repository to my own account (https://github.com/seisman/pygmt), and made changes directly to my own master branch.

If you compare my master branch and the PyGMT master branch, you will see my master branch is 2 commits ahead of PyGMT master branch. The two commits are:

  • seisman@11446e2: This commit is the same as the only commit in this PR
  • seisman@37f83e4: The slash command uses the GenericMappingTools bot to generate a token, which is not available in my own fork. So I have to use my own token instead. This commit is necessary in my own fork for testing purposes.

To confirm that everything works well, I opened an example PR in my own fork (seisman#2). Please go to that PR to see how it works.

TODO:

Known issues:

The "GMT Latest Tests" triggered by /test-gmt-master won't show the workflow status in the status summary page (like the screenshot below), so we have to go the the "Actions" page to find the testing results.
image

Note: you may find the following status page in PR seisman#2. These "GMT Latest Tests" status were triggered by "mark it ready for review", not by the slash command /test-gmt-master.
image

One possible solution is: when the workflow is triggered by slash commands, we can let the workflow leave a comment saying that "The workflow is running at https://github.com/GenericMappingTools/pygmt/actions/runs/XXXXX"

Fixes #828.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Notes

  • You can write /format in the first line of a comment to lint the code automatically

@seisman seisman added the maintenance Boring but important stuff for the core devs label Feb 1, 2021
@seisman seisman added this to the 0.3.0 milestone Feb 1, 2021
@seisman seisman requested a review from weiji14 February 1, 2021 06:55
weiji14
weiji14 reviewed Feb 1, 2021
View reviewed changes
Copy link
Member

@weiji14 weiji14 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please also update the part in https://github.com/GenericMappingTools/pygmt/blob/v0.2.1/MAINTENANCE.md#github-actions which says:

ci_tests_dev.yaml (GMT Latest Tests on Linux/macOS/Windows).

This is only triggered when a review is requested or re-requested on a PR. It is also scheduled to run daily on the master branch.

to mention that the test can also be triggered using /test-gmt-master.

Note: you may find the following status page in PR seisman#2. These "GMT Latest Tests" status were triggered by "mark it ready for review", not by the slash command /test-gmt-master.
image
One possible solution is: when the workflow is triggered by slash commands, we can let the workflow leave a comment saying that "The workflow is running at https://github.com/GenericMappingTools/pygmt/actions/runs/XXXXX"

Might be complicated trying to code up the chatbot comment (though it will be nice, if not a bit annoying). How often will we need to keep triggering GMT Latest Tests? If it's not too often I think it's fine to just navigate to the Actions tab manually.

@seisman
Copy link
Member Author

seisman commented Feb 1, 2021 •
edited
Loading

Might be complicated trying to code up the chatbot comment (though it will be nice, if not a bit annoying). How often will we need to keep triggering GMT Latest Tests? If it's not too often I think it's fine to just navigate to the Actions tab manually.

We probably only want to trigger the GMT Latest Tests when a PR is close to merging. We can navigate to the Actions tab manually and see how it goes.

@seisman seisman requested a review from weiji14 February 1, 2021 21:03
weiji14
weiji14 reviewed Feb 1, 2021
View reviewed changes
.github/workflows/slash-command-dispatch.yml Outdated
token: ${{ steps.generate-token.outputs.token }}
commands: |
format
test-gmt-master
Copy link
Member

@weiji14 weiji14 Feb 1, 2021 •
edited by seisman
Loading

Choose a reason for hiding this comment

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

Suggested change
test-gmt-master
test-gmt-dev

What do you think about changing the name to 'latest' to match the Github Actions name?

Copy link
Member Author

@seisman seisman Feb 1, 2021

Choose a reason for hiding this comment

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

When we say "the latest GMT", we usually mean "the latest stable GMT release" (i.e., 6.1.1).

What about "GMT Dev Tests" and /test-gmt-dev?

Copy link
Member

@weiji14 weiji14 Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, dev sounds good and is shorter. Please remember to change the PR title too.

Copy link
Member Author

@seisman seisman Feb 1, 2021

Choose a reason for hiding this comment

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

Done in 6df8e34 and 72b7dfa

@seisman seisman changed the title Add slash command '/test-gmt-master' to test GMT master branch Add slash command '/test-gmt-dev' to test GMT master branch Feb 1, 2021
seisman
seisman commented Feb 1, 2021
View reviewed changes
@seisman @weiji14
Apply suggestions from code review
6df8e34 
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@seisman seisman changed the title Add slash command '/test-gmt-dev' to test GMT master branch Add slash command '/test-gmt-dev' to test GMT dev version Feb 2, 2021
@seisman seisman merged commit 213a414 into master Feb 2, 2021
@seisman seisman deleted the slash-test-gmt branch February 2, 2021 00:31
@seisman seisman mentioned this pull request Feb 28, 2021
Merged
5 tasks
@weiji14 weiji14 mentioned this pull request Oct 16, 2021
Merged
5 tasks
@weiji14 weiji14 mentioned this pull request Mar 14, 2022
Closed
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
@seisman @weiji14
Add slash command '/test-gmt-dev' to test GMT dev version (GenericMap…
3e3ffe1 
…pingTools#831)

* Add slash command '/test-gmt-dev' to test GMT dev version
* Remove 'review_requested' from the triggering condition
* Update pull request template
* Update maintenance guide
* Rename workflow to 'GMT Dev Tests'

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@weiji14 weiji14 mentioned this pull request Jan 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weiji14 weiji14 weiji14 approved these changes

Assignees

No one assigned

Labels

maintenance Boring but important stuff for the core devs

Projects

None yet

Milestone

0.3.0

Development

Successfully merging this pull request may close these issues.

The workflow of "GMT Latest Tests" are triggered multiple times

2 participants

@seisman @weiji14