-
Notifications
You must be signed in to change notification settings - Fork 0
ODP-5569: Enable creating releases even if the 'previous' tag does not exist #8
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
Conversation
Not all of our repositories always have the matching previous tag, so while creating a new release, these components would fail due to the missing previous tag. This tag is used for generating release notes; rather than always have them fail, a cli option has been added `skip-previous-tag` which allows you to skip generating release notes for repositories where they cannot be generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a --skip-missing-tag flag to enable creating releases even when the previous tag doesn't exist in some repositories. This addresses a scenario where new components added to version 3.2.3.4-{2,3} don't have tags from 3.2.3.3-{2,3}, allowing releases to be created without release notes rather than failing entirely. The PR also includes widespread refactoring to replace .to_string() calls with .clone() for performance optimization when working with String types.
Key Changes:
- Added
--skip-missing-tagCLI flag that allows release creation without requiring the previous tag to exist - Modified release creation logic to gracefully handle missing previous tags by creating releases with empty release notes
- Refactored numerous
.to_string()calls to.clone()across the codebase for better performance - Updated documentation with examples of the new flag usage
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli.rs | Added skip_missing_tag boolean field and made previous_release optional when this flag is present |
| src/github/release.rs | Modified release creation to handle missing previous tags gracefully, added error handling logic and comments, fixed typo in comment |
| src/github/match_args.rs | Updated to pass skip_missing_tag parameter to release functions and handle optional previous_release |
| README.md | Added documentation explaining the --skip-missing-tag flag with usage examples |
| src/github/tag.rs | Refactored .to_string() to .clone() for String types |
| src/github/pr.rs | Refactored .to_string() to .clone() for String types |
| src/github/fork.rs | Refactored .to_string() to .clone() for String types |
| src/github/branch.rs | Refactored .to_string() to .clone() for String types |
| src/github/backup.rs | Refactored .to_string() to .clone() for String types |
| src/slack/post_message.rs | Refactored .to_string() to .clone() for webhook_url parameter |
| src/error.rs | Refactored .to_string() to .clone() for error message handling |
| Cargo.toml | Bumped version from 1.0.0 to 1.0.1 |
| Cargo.lock | Updated version lock to 1.0.1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The previous release tag. This is used to generate the changelog | ||
| #[arg(short, long)] | ||
| pub previous_release: String, | ||
| #[arg(short, long, required_unless_present = "skip_missing_tag")] |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI allows previous_release to be omitted when skip_missing_tag is true (via required_unless_present), but the documentation example (README.md line 383) still shows both flags being used together. Based on the PR description indicating that "some repositories do not have a valid 'previous' tag," the intention seems to be handling missing tags in specific repos, not omitting the previous release parameter entirely.
When previous_release is None and defaults to an empty string (line 553), it will be passed to generate_release_notes() which attempts to fetch a tag with an empty name, causing unclear failures. Consider either:
- Keeping
previous_releaseas required (removingrequired_unless_present), or - Handling empty/None
previous_releaseexplicitly in the release functions by skipping tag comparison entirely when it's not provided.
| #[arg(short, long, required_unless_present = "skip_missing_tag")] | |
| #[arg(short, long, required = true)] |
src/github/branch.rs
Outdated
| let old_text_dash = &old_text.clone().replace('.', "-"); | ||
| let new_text_dash = &new_text.clone().replace('.', "-"); |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .clone() call is unnecessary here. The .replace() method takes &self and returns a new String, so calling .clone() before .replace() creates an unnecessary intermediate copy. Since old_text and new_text are already owned String values, you can simply use &old_text.replace('.', "-") and &new_text.replace('.', "-").
| let old_text_dash = &old_text.clone().replace('.', "-"); | |
| let new_text_dash = &new_text.clone().replace('.', "-"); | |
| let old_text_dash = &old_text.replace('.', "-"); | |
| let new_text_dash = &new_text.replace('.', "-"); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ReleaseCommand::Create(create_cmd) => { | ||
| let repository = create_cmd.repository.as_ref(); | ||
| let skip_missing_previous_tag = create_cmd.skip_missing_tag; | ||
| let previous_release = create_cmd.previous_release.clone().unwrap_or_default(); |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using .unwrap_or_default() on previous_release results in an empty string "" being passed to the release creation functions when no previous release is specified. While this works because the error is caught and handled when skip_missing_tag is true, it would be clearer to use a more explicit empty tag placeholder like "NONE" or to handle the Option<String> directly throughout the call chain. This would make the code's intent more obvious and avoid attempting to fetch a tag with an empty name.
src/github/release.rs
Outdated
| let release_notes = if let Ok(notes) = release_notes { | ||
| notes | ||
| } else if ignore_previous_tag { | ||
| eprintln!( | ||
| "The previous tag '{previous_tag}' does not exist. Attempting to create a release for '{owner}/{repo}' without release notes." | ||
| ); | ||
| ReleaseNotes { | ||
| name: current_tag.to_string(), | ||
| body: String::new(), | ||
| } | ||
| } else { | ||
| return Err(GitError::NoSuchTag(previous_tag.to_string())); |
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling on lines 241-253 catches any error from generate_release_notes, not just errors related to missing previous tags. This means that if there's a network error, API error, or the current tag doesn't exist, the function will still proceed to create a release without notes when ignore_previous_tag is true. Consider checking the specific error type and only ignoring GitError::NoSuchTag errors for the previous tag, while propagating other errors.
| let release_notes = if let Ok(notes) = release_notes { | |
| notes | |
| } else if ignore_previous_tag { | |
| eprintln!( | |
| "The previous tag '{previous_tag}' does not exist. Attempting to create a release for '{owner}/{repo}' without release notes." | |
| ); | |
| ReleaseNotes { | |
| name: current_tag.to_string(), | |
| body: String::new(), | |
| } | |
| } else { | |
| return Err(GitError::NoSuchTag(previous_tag.to_string())); | |
| let release_notes = match release_notes { | |
| Ok(notes) => notes, | |
| Err(GitError::NoSuchTag(tag)) if ignore_previous_tag && tag == previous_tag => { | |
| eprintln!( | |
| "The previous tag '{previous_tag}' does not exist. Attempting to create a release for '{owner}/{repo}' without release notes." | |
| ); | |
| ReleaseNotes { | |
| name: current_tag.to_string(), | |
| body: String::new(), | |
| } | |
| } | |
| Err(e) => return Err(e), |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
We ran into an issue while creating releases for 3.2.3.4-{2,3} where some repositories do not have a valid 'previous' tag, while some did. Certain components have been added to 3.2.3.4-{2,3} that were not previously there in 3.2.3.3-{2,3}, so when a release was created, those components' releases failed instead of being created but without the release notes.
This commit adds a
--skip-missing-tagcli flag to allow ignoring failures in the release note generation code and attempt to create the release without them.This has been tested both for a single repository and a group with at least one repository missing the previous tag, as well as checking that this did not break any previous functionality.