-
Notifications
You must be signed in to change notification settings - Fork 124
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
The package available via dnf is in a good place #879
Conversation
Reviewer's Guide by SourceryThis pull request introduces Sequence diagram for ramalama installation with dnfsequenceDiagram
participant User
participant install.sh
participant dnf
participant GitHub
User->>install.sh: Executes install.sh
install.sh->>install.sh: Checks if dnf is available
alt dnf is available
install.sh->>dnf: Installs python3-ramalama
alt Installation successful
dnf-->>install.sh: Success
install.sh-->>User: ramalama installed via dnf
else Installation failed
dnf-->>install.sh: Failure
install.sh->>GitHub: Downloads ramalama files
GitHub-->>install.sh: ramalama files
install.sh->>install.sh: Installs ramalama from files
install.sh-->>User: ramalama installed from files
end
else dnf is not available
install.sh->>GitHub: Downloads ramalama files
GitHub-->>install.sh: ramalama files
install.sh->>install.sh: Installs ramalama from files
install.sh-->>User: ramalama installed from files
end
Flow diagram for ramalama installation processgraph TD
A[Start install.sh] --> B{dnf available?}
B -- Yes --> C{Install python3-ramalama with dnf}
C -- Success --> D[ramalama installed]
C -- Fail --> E[Download ramalama from GitHub]
B -- No --> E
E --> F[Install ramalama from files]
F --> D
File-Level Changes
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 @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the
dnf install
command might fail. - It might be better to move the core logic of installing the python files into a separate function, and call that function from both the dnf and non-dnf paths.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
Defaulting to that on platforms that have dnf, if it fails for whatever reason, fall back to this script. Signed-off-by: Eric Curtin <ecurtin@redhat.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.
LGTM
Defaulting to that on platforms that have dnf, if it fails for whatever reason, fall back to this script.
Summary by Sourcery
This pull request modifies the installation script to first attempt to install
ramalama
usingdnf
if it is available. Ifdnf
is not available or the installation fails, the script falls back to the existing installation method. The installation process is also refactored into separate functions for installing the binary and libraries.New Features:
dnf
package manager on systems where it is available.Enhancements: