Skip to content

Restore sanity check for gh tool#1444

Open
scpeters wants to merge 2 commits intomasterfrom
scpeters/fix_gh_check
Open

Restore sanity check for gh tool#1444
scpeters wants to merge 2 commits intomasterfrom
scpeters/fix_gh_check

Conversation

@scpeters
Copy link
Contributor

@scpeters scpeters commented Feb 13, 2026

A sanity check for presence of the gh tool was recently added in #1436 and reverted in #1443. This is an attempt to restore the sanity check by only checking for gh in cases when a ROS vendor package PR may be opened by process_ros_vendor_package().

This should be tested before merging.

j-rivero and others added 2 commits February 13, 2026 10:15
Add a sanity check to verify that the GitHub CLI (gh) tool is available
in PATH before proceeding with the release. This prevents the release
process from failing late in the process when gh is needed for creating
PRs in vendor repositories.

Fixes #1433

Generated-by: Claude Opus 4.5
The built-in jenkins executor that runs the _releasepy job
doesn't have gh installed, causing it to fail the sanity
check. This runs the gh sanity check only in cases when
a ROS vendor package PR may be opened.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from azeey February 13, 2026 18:34
@scpeters scpeters requested a review from j-rivero as a code owner February 13, 2026 18:34
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

@scpeters
Copy link
Contributor Author

scpeters commented Feb 13, 2026

Worked on https://build.osrfoundation.org/job/_releasepy/307/console

I think that uses the master branch of release-tools

https://build.osrfoundation.org/job/_releasepy/307/parameters/

@azeey
Copy link
Contributor

azeey commented Feb 13, 2026

You're right. I only checked out your branch locally. Not sure what is a good way to test this

@scpeters
Copy link
Contributor Author

You're right. I only checked out your branch locally. Not sure what is a good way to test this

there are multiple contexts in which release.py gets called:

  1. Called by a package maintainer to tag the repo and trigger a -source build (step 1 in the release process documentation)
  2. Called by the _releasepy jenkins job (see OSRFReleasepy.groovy and step 4 in the release process documentation)
  3. Called by the -nightly-scheduler jenkins jobs (like https://build.osrfoundation.org/job/ignition-jetty-nightly-scheduler, which has actually been failing Build Status for the past few days, I guess it's not in the build farmer dashboard)

@scpeters
Copy link
Contributor Author

  1. Called by the -nightly-scheduler jenkins jobs (like https://build.osrfoundation.org/job/ignition-jetty-nightly-scheduler, which has actually been failing Build Status for the past few days, I guess it's not in the build farmer dashboard)

to test the nightly scheduler, I first created another "broken" branch from 8fb24af (without the fix in e1c2ef3) and ran a dry-run nightly scheduler job, which correctly failed: Build Status

then I ran a dry-run nightly scheduler from this branch and it passed: Build Status

@scpeters
Copy link
Contributor Author

  1. Called by a package maintainer to tag the repo and trigger a -source build (step 1 in the release process documentation)

to test this, someone should try a dry-run release.py invocation without gh installed on your local system

  1. Called by the _releasepy jenkins job (see OSRFReleasepy.groovy and step 4 in the release process documentation)

to test this, I think we would need to run a -source job with the proper RTOOLS_BRANCH parameter, since I believe the downstream _releasepy job would inherit that parameter

@scpeters
Copy link
Contributor Author

  1. Called by the _releasepy jenkins job (see OSRFReleasepy.groovy and step 4 in the release process documentation)

to test this, I think we would need to run a -source job with the proper RTOOLS_BRANCH parameter, since I believe the downstream _releasepy job would inherit that parameter

if we added --rtools-branch as a parameter to release.py, we could copy it to params["RTOOLS_BRANCH"] where the other params are set

# If only the process of ROS vendor package is set, just do it
if args.bump_ros_vendor_only:
# gh is needed for ROS vendor package PR
sanity_check_gh_tool()
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the implementation works, I would prefer to avoid calling the sanity checks in the main logic and do that unconditionally at the begging. The sanity check should know if it needs to be applied or skipped. Something like master...jrivero/restore_gh_check .

I think that is going to be easier to maintain and save some lines to read in the logic.

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.

3 participants