Add release helper #284
Add release helper #284
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Note: this was created with a WIP tool that keeps versions in check across our rust/python/npm packages, which can be found in the [paulgb/releaser](#284) branch.
paulgb
force-pushed
the
paulgb/releaser
branch
from
4de51f2 to
fee734f
Compare
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a series of changes across multiple files in the repository, primarily focused on enhancing the release management process. A new Rust package, New modules for managing packages in Rust, Node.js, and Python have been added, along with functionality in Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
Outside diff range and nitpick comments (8)
releaser/src/check_binaries.rs (1)
24-36: LGTM with a suggestion!The
check_binariesfunction is implemented correctly:
- It takes a
Versionas input and returns aResult<bool>.- Converts the version to a string to construct the URLs.
- Iterates over the hardcoded list of binary names.
- Constructs the URL for each binary using the hardcoded URL prefix and version.
- Calls the
is_url_okfunction to check if the URL returns a 200 status code.- Returns false if any binary is not found, which is the correct behavior.
- Returns true only if all binaries are found.
- Error handling is done using the
Resulttype.Consider adding a comment above the
BINARIES_TO_CHECKandBINARY_URL_PREFIXconstants mentioning that they need to be updated if the list of binaries or the URL format changes in the future. This will serve as a reminder for developers modifying this code.releaser/src/util.rs (2)
5-17: LGTM!The function logic is correct, and the implementation is accurate. Great job using the
dialoguercrate to simplify the user input process.Consider the following suggestions for further improvement:
- Return a
Resultinstead of unwrapping theinteract()call to handle potential errors gracefully.- Take a reference to the vector instead of moving it to avoid unnecessary memory allocation.
56-67: LGTM!The function logic is correct, and the implementation is accurate. It's a useful utility function for finding the root directory of the project.
Consider the following suggestions for further improvement:
- Return a
Resultinstead of panicking to handle the case where the root directory is not found gracefully.- Take an optional starting path as an argument to allow searching from a specific directory.
releaser/README.md (1)
16-18: Consider specifying languages for fenced code blocks.To improve syntax highlighting and readability, consider specifying the language for the fenced code blocks. For example:
-``` +```bash cargo run -- bump-
+bash
cargo run -- publishAlso applies to: 43-45 <details> <summary>Tools</summary> <details> <summary>Markdownlint</summary><blockquote> 16-16: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </blockquote></details> </details> </blockquote></details> <details> <summary>releaser/src/package_manager/mod.rs (1)</summary><blockquote> `81-103`: **Consider caching `PackageManager` instances to improve performance.** Each method in `Package` creates a new `PackageManager` instance. If these instances are stateless, this may be acceptable. However, if they maintain state or if instantiation is resource-intensive, caching them within the `Package` struct can enhance efficiency. You might modify the `Package` struct to include a `package_manager` field: ```diff pub struct Package { pub name: String, pub path: PathBuf, pub package_type: PackageType, + package_manager: Box<dyn PackageManager>, }And initialize it accordingly, reducing the need to create a new instance with every method call.
releaser/src/package_manager/python/mod.rs (2)
71-71: Clarify the error message when updating dependenciesThe error message
"Python dependencies not supported"might be unclear. Consider rephrasing it to"Updating Python dependencies is not supported"to provide better context.Apply this diff to update the error message:
-return Err(anyhow::anyhow!("Python dependencies not supported")); +return Err(anyhow::anyhow!("Updating Python dependencies is not supported"));
21-26: Include HTTP status in error message when fetching published versionAdding the HTTP status code to the error message can provide more context about why the request failed.
Apply this diff to enhance the error message:
return Err(anyhow::anyhow!( - "Failed to get public version for package {}", - package + "Failed to get public version for package {}: HTTP {}", + package, + response.status() ));releaser/src/package_manager/cargo.rs (1)
155-159: Avoid Deserializing Unused Fields to Optimize PerformanceThe fields
newest_versionandmax_stable_versionare marked with#[allow(unused)]. If these fields are not used, you can omit them from the struct to simplify the code and slightly improve deserialization performance.Apply this diff to exclude unused fields:
#[derive(Debug, Deserialize)] struct CratesCrate { - #[allow(unused)] - newest_version: Version, max_version: Version, - #[allow(unused)] - max_stable_version: Version, }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
releaser/Cargo.lockis excluded by!**/*.lock
Files selected for processing (16)
- .github/workflows/release.yml (1 hunks)
- python/pyproject.toml (1 hunks)
- releaser/.gitignore (1 hunks)
- releaser/Cargo.toml (1 hunks)
- releaser/README.md (1 hunks)
- releaser/src/check_binaries.rs (1 hunks)
- releaser/src/git.rs (1 hunks)
- releaser/src/main.rs (1 hunks)
- releaser/src/package_manager/cargo.rs (1 hunks)
- releaser/src/package_manager/mod.rs (1 hunks)
- releaser/src/package_manager/node.rs (1 hunks)
- releaser/src/package_manager/python/mod.rs (1 hunks)
- releaser/src/package_manager/python/virtualenv.rs (1 hunks)
- releaser/src/packages.rs (1 hunks)
- releaser/src/releaser.rs (1 hunks)
- releaser/src/util.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/release.yml
- releaser/.gitignore
Additional context used
Markdownlint
releaser/README.md
12-12: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
29-29: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
37-37: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
16-16: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
43-43: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
Additional comments not posted (26)
releaser/Cargo.toml (1)
1-18: LGTM!The
Cargo.tomlfile is well-structured and includes all the necessary package metadata and dependencies. The dependencies have specific version constraints to ensure compatibility, and some dependencies enable specific features that are likely needed by the project.The chosen dependencies suggest a well-thought-out design for the release helper tool, including:
clapfor command-line argument parsinggit2for interacting with Git repositoriessemverfor version parsing and comparisonserdeandserde_jsonfor JSON serialization and deserializationtomlandtoml_editfor reading and writing TOML filesOverall, the package configuration and dependencies look good to me!
python/pyproject.toml (2)
27-27: LGTM!Adding the
buildpackage as a dev dependency is a good practice for building and distributing the package. Pinning the version ensures reproducible builds.
29-29: LGTM!Adding the
twinepackage as a dev dependency is a good practice for publishing the package. Using the~=version specifier allows for compatible releases while avoiding potential breaking changes.releaser/src/check_binaries.rs (1)
17-21: LGTM!The
is_url_okfunction is implemented correctly:
- It creates a new blocking reqwest client.
- Sends a HEAD request to the provided URL.
- Returns true if the response status is in the success range.
- The function signature and return type are appropriate.
- Error handling is done using the
Resulttype.releaser/src/packages.rs (2)
7-10: LGTM!The struct definition is clear and uses appropriate types for the fields.
52-54: LGTM!The
itermethod provides a convenient way to iterate over the packages in thePackageList.releaser/src/util.rs (1)
19-54: LGTM!The
BumpTypeenum is well-defined and follows Rust conventions. TheDisplaytrait implementation and thebumpassociated function are implemented correctly.releaser/README.md (3)
12-27: LGTM!The instructions for bumping the version are clear, accurate, and consistent with the expected process. The step-by-step guidance and the provided command will be helpful for developers.
Tools
Markdownlint
12-12: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
16-16: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
29-36: Instructions are clear and informative.The instructions for creating a release through GitHub Actions are well-written and provide the necessary information for developers. The link to the release workflow, the note about the version number, and the description of the expected outcome make the process easy to understand and follow.
Tools
Markdownlint
29-29: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
37-50: Publishing instructions are well-documented.The instructions for publishing packages are clear and provide the necessary information for developers. The note about the package referring to the published binaries, the command to run the releaser program, and the list of steps performed by the program make the process easy to understand and follow.
Tools
Markdownlint
37-37: Expected: setext; Actual: atx
Heading style(MD003, heading-style)
43-43: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
releaser/src/package_manager/python/virtualenv.rs (4)
8-13: LGTM!The function logic is correct, and the implementation is accurate. Using a random u64 value ensures a high probability of generating a unique directory path.
18-21: LGTM!The struct definition is clear and well-documented. The fields are appropriately named and typed.
24-51: LGTM!The method logic is correct, and the implementation is accurate. It properly creates a new virtual environment, installs the specified dependencies, and returns a new instance of
VirtualEnv.
54-56: LGTM!The method is a simple getter that returns the path to the Python executable. The implementation is correct.
releaser/src/git.rs (5)
11-14: LGTM!The function logic is correct, and the implementation is accurate. Using
Resultfor error handling is a good practice.
17-28: LGTM!The function logic is correct, and the implementation is accurate. Using
Resultfor error handling is a good practice.
30-38: LGTM!The function logic is correct, and the implementation is accurate. Using
Resultfor error handling andanyhow::bail!for custom errors is a good practice.
41-49: LGTM!The function logic is correct, and the implementation is accurate. Using the git CLI for staging and committing changes is a reasonable approach, as it leverages the user's git configuration.
52-60: LGTM!The function logic is correct, and the implementation is accurate. Using
Resultfor error handling is a good practice.releaser/src/main.rs (5)
1-13: LGTM!The import statements and module declarations are well-structured and follow Rust's naming conventions. The usage of external crates and internal modules is appropriate for the project's functionality.
14-31: LGTM!The command-line arguments are well-defined using the
clapcrate. TheBumpandPublishsubcommands provide a clear interface for the release process. The optionalversionargument for theBumpsubcommand allows flexibility in manual version updates.
32-51: LGTM!The
mainfunction sets up thePackageListwith various packages in a clear and structured manner. The comment about the publishable order is helpful for maintaining the correct release sequence. The registration of packages using descriptive names and paths improves the readability and maintainability of the code.
52-58: LGTM!The creation of the
Releaserinstance with thePackageListencapsulates the release process effectively. The parsing of command-line arguments usingArgs::parse()is consistent with theclapcrate's API. Thematchexpression provides a clear way to handle the different subcommands and their respective actions.
76-77: LGTM!The closing braces for the
matchexpression and themainfunction are in the correct position, and there are no syntax issues.releaser/src/package_manager/cargo.rs (2)
116-122: Correct Logical Error in Dependency Version UpdateIn the
update_dep_tablefunction, there's a logical error where the existing version is reassigned instead of updating to the new version. The variableversioninside theif letblock shadows the function parameterversion.Apply this diff to fix the version assignment:
if let Some(_) = dep_version.as_str() { - *dep_version = value(version.to_string()); + *dep_version = value(version.to_string()); } else if let Some(table) = dep_version.as_table_like_mut() { - table.insert("version", value(version.to_string())); + table["version"] = value(version.to_string()); } else { continue; };Make sure to remove the inner variable
versionto prevent shadowing and assign the new version correctly.Likely invalid or redundant comment.
31-36: Handle Potential Parsing Errors in Response JSONIf the response from crates.io does not contain the expected fields, the code may fail when parsing. Consider adding error handling for missing or malformed data.
Run the following script to verify the structure of the response:
Ensure that the
max_versionfield exists and is in the expected format.Verification successful
API Response Structure Verified, Existing Error Handling Sufficient
The crates.io API response contains the expected
max_versionfield in the correct location, matching the structure used in the code. The existing error handling (usingcontext()and the?operator) should adequately handle parsing issues for most cases.If extremely robust error handling is desired, consider adding:
- Validation of the
max_versionstring to ensure it's a valid semantic version.- A check for the presence of the
_crateandmax_versionfields before accessing them.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the crates.io API response contains the expected fields. # Test: Fetch the package data and check for 'max_version' field. # Replace 'package_name' with an actual package name for testing. curl -s https://crates.io/api/v1/crates/package_name | jq '.crate.max_version'Length of output: 86
paulgb
force-pushed
the
paulgb/releaser
branch
from
80ca67a to
2ce5c30
Compare
|
Left some comments re: some steps that were missed from this doc: https://www.notion.so/jamsocket/Y-sweet-release-checklist-e2d9fc60c0c143a2aa8ad2ddd4964e73 |
releaser/src/releaser.rs
Outdated
| } | ||
|
|
||
| pub fn bump(&self, version: Option<Version>) -> Result<()> { | ||
| ensure_repo_ready(&self.git)?; | ||
| if !self.allow_dirty { | ||
| ensure_repo_ready(&self.git)?; |
There was a problem hiding this comment.
nit: should still probably never allow uncommitted changes (it's okay if the changes have been committed but aren't yet on main, though)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
paulgb
force-pushed
the
paulgb/releaser
branch
from
3c37b98 to
c5ee54c
Compare
This PR introduces a Rust program that helps with keeping versions in sync across our rust/python/JS codebases, both in the repo and in their respective public registries.
See
releaser/README.mdfor instructions on usage. This replaces the notion checklist (note: the current checklist is only accessible to Jamsocket notion workspace members) we previously followed, as well asset-js-version.tswhich was removed in #285.Instructions are in the README.
The goal is to reduce the chances of human error and reduce the toil that comes from managing multiple packages across several registries.