-
Notifications
You must be signed in to change notification settings - Fork 115
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
--version points to git hash #756
Conversation
Reviewer's Guide by SourceryThe PR updates the version reporting mechanism to display git commit information by reading a version file, rather than relying on package metadata. The changes include refactoring in the version.py file to read the version from a configuration file and adding logic in the install.sh file to generate this version file based on git describe output. Sequence diagram for version reporting mechanismsequenceDiagram
actor User
participant PrintVersion as print_version()
participant VersionFunc as version()
participant FS as FileSystem
User->>PrintVersion: Run 'ramalama --version'
PrintVersion->>VersionFunc: Call version()
VersionFunc->>FS: Check if version file exists
alt File exists
FS-->>VersionFunc: Return file content
VersionFunc-->>PrintVersion: Return version string
else File does not exist
VersionFunc-->>PrintVersion: Return error message with file path
end
PrintVersion->>User: Print "ramalama version <version>"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @dougsland - I've reviewed your changes - here's some feedback:
Overall Comments:
- In setup_version_file, assign a default version when not in a git repository to avoid writing an empty string to the version file.
- Consider handling cases where the regex match fails so that unexpected git describe formats don't lead to unformatted version strings.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Today ramalama just share version 0.5.5 (as example) but don't tell users and developer which commit it's using. Resolves: containers#754 Signed-off-by: Douglas Schilling Landgraf <[email protected]>
@@ -100,6 +100,29 @@ check_platform() { | |||
return 0 | |||
} | |||
|
|||
setup_version_file() { |
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.
This would not work in the pypi install, rpm install, or any install that does not use the install.sh.
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.
Could we simply use a setup.py.in
with a placeholder for the version, i.e. version=@VERSION@
, which we can replace at build-time? This could then be used by all means to install ramalama by simply adding this prepare step. Additionally, the version would be hard-coded and uses the git hash only for non-release builds. We use that approach for example in BlueChi
WDYT? @dougsland @rhatdan
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.
In order to get PRs merged, you need to modify the tests to make sure they pass. |
Why do you care about a git hash? We should be only cutting releases, and then you can go to github.com and look up the release which will point at the github hash? |
It's generally useful to include version details in the --version flag so users when visit the release page and can match the commit or just locally via git. Additionally, during development, having the version information helps track which patch is being used, especially when working with multiple patches. After all, when developing it always says v0.5.5 which is not really correct it's release v0.5.5 + current git commit. However, I am okay to close this, if you and guys decide, it's just something I found during my tests+development. |
If you are developing you can just do git log? |
Today ramalama just share version 0.5.5 (as example) but don't tell users and developer which commit it's using.
Resolves: #754
Summary by Sourcery
Update the version reporting to use the Git commit hash if available, falling back to a file-based version if not in a Git repository.
New Features:
Enhancements: