Conversation
bump_packaging
Outdated
| fi | ||
|
|
||
| git commit -m "Release $FULLVERSION" | ||
| git push "$PACKAGING_GIT_REMOTE" "HEAD:$REMOTE_BRANCH" |
There was a problem hiding this comment.
This pushes straight to the target branch. A PR after this is pointless because it's effectively already merged.
Remember that phr is both a push (to your own fork) and a PR.
There was a problem hiding this comment.
This pushes straight to the target branch. A PR after this is pointless because it's effectively already merged.
Remember that
phris both a push (to your own fork) and a PR.
I have missed the fact that $PACKAGING_GIT_REMOTE is the upstream repository. I'll fix it in the next patch.
|
I've pushed updates that address the issues identified in the review. Changes I've made:
Benefits:
|
|
I initially considered using just
|
|
I've used the script to bump rpm/3.16 to 3.16.0. Here is the script output if anyone is interested. One thing can be improved, though. That is the |
settings
Outdated
| GIT_DEVELOP_BRANCH=develop | ||
| GIT_STABLE_BRANCH="${VERSION}-stable" | ||
| GITHUB_NAMESPACE=theforeman | ||
| GITHUB_USER="${GITHUB_USER:-$(gh api user --jq .login)}" |
There was a problem hiding this comment.
This performs a network request and I'd suggest to avoid that. Perhaps just assume the GITHUB_USER is $USER?
There was a problem hiding this comment.
Since we're including gh in the script, the network requests are going to be performed anyway
There was a problem hiding this comment.
Not always. Remember that settings is always loaded, also for commands where git isn't involved.
There was a problem hiding this comment.
True. In that case, there's also an option to keep this API call but do it only when needed.
There was a problem hiding this comment.
I'd still lean to keeping it simple:
| GITHUB_USER="${GITHUB_USER:-$(gh api user --jq .login)}" | |
| GITHUB_USER="${GITHUB_USER:-$USER}" |
Then users can override it in their local settings if they want. Either statically or dynamically with gh api user.
98f6be4 to
56c9e72
Compare
|
@ekohl Mind revisiting? I can confirm the patch works nicely, I've used it to create |
settings
Outdated
| GITHUB_USER="${GITHUB_USER:-$USER}" | ||
| PACKAGING_DIR="$GIT_DIR/foreman-packaging" | ||
| PACKAGING_GIT_REMOTE=origin | ||
| PACKAGING_FORK_REMOTE="${PACKAGING_FORK_REMOTE:-origin}" |
There was a problem hiding this comment.
These both default to origin, but is that really what we want? You'll end up running gh repo fork theforeman/foreman-packaging --remote-name "origin" but there is already an origin. Doesn't it make more sense to default to the GH username?
| PACKAGING_FORK_REMOTE="${PACKAGING_FORK_REMOTE:-origin}" | |
| PACKAGING_FORK_REMOTE="${PACKAGING_FORK_REMOTE:-${GITHUB_USER}}" |
There was a problem hiding this comment.
That totally makes sense. Addressed.
bump_packaging
Outdated
| if [[ $PACKAGING_PR == true ]] ; then | ||
| git phr -m "Release $FULLVERSION" -b "$REMOTE_BRANCH" | ||
| # Ensure fork exists with configured remote name | ||
| gh repo fork theforeman/foreman-packaging --remote-name "$PACKAGING_FORK_REMOTE" |
There was a problem hiding this comment.
When I try this out locally I see:
$ gh repo fork theforeman/foreman-packaging --remote-name ekohl
! ekohl/foreman-packaging already exists
? Would you like to clone the fork? (y/N) So this is an interactive command and that goes against the spirit of the tooling. I don't see any option to make gh non-interactive and quiet.
There was a problem hiding this comment.
That's a good point. I stumbled upon GH_PROMPT_DISABLED
GH_PROMPT_DISABLED: set to any value to disable interactive prompting in the terminal.
$ GH_PROMPT_DISABLED=1 gh repo fork theforeman/pulpcore-packaging --remote-name ekohl
✓ Created fork ogajduse/pulpcore-packaging
$ echo $?
0
$ GH_PROMPT_DISABLED=1 gh repo fork theforeman/pulpcore-packaging --remote-name ekohl
! ogajduse/pulpcore-packaging already exists
$ echo $?
0
Updated.
bump_packaging
Outdated
| # Ensure fork exists with configured remote name | ||
| gh repo fork theforeman/foreman-packaging --remote-name "$PACKAGING_FORK_REMOTE" | ||
|
|
||
| # Push to fork |
There was a problem hiding this comment.
This may be nitpicking, but does the comment really add anything? I'd think that the command it self-explanatory.
There was a problem hiding this comment.
Agreed. Comment removed.
- Replace git phr alias with gh pr create in bump_packaging script - Change push behavior to always push before creating PR - Add GitHub CLI installation documentation to README - Addresses git phr dependency issue mentioned in theforeman#336 This removes the dependency on custom git aliases and uses the standard GitHub CLI tool that's available in distribution packages.
- Add GITHUB_USER setting with auto-detection via gh CLI - Add PACKAGING_FORK_REMOTE setting for configurable fork remote name - Fix git push/PR creation logic in bump_packaging: - Remove direct push to target branch when creating PRs - Use deterministic branch names: release-$PROJECT-$FULLVERSION - Explicitly manage fork creation with gh repo fork - Use configured GITHUB_USER and PACKAGING_FORK_REMOTE settings - Document new settings in README - Ensures predictable behavior and reusable fork setup
- Change PACKAGING_FORK_REMOTE default from 'origin' to GITHUB_USER - Simplify git push to use --set-upstream with plain HEAD - Update documentation to reflect actual default behavior
e181d42 to
0a27c76
Compare
|
Rebased onto master |
Summary
Replaces the custom
git phralias dependency with the standard GitHub CLI tool.Changes
bump_packagingto usegh pr createinstead ofgit phrBenefits