-
Notifications
You must be signed in to change notification settings - Fork 1
Add EESSI CVMFS config to package repositories #448
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
Summary of ChangesHello @bertiethorpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the necessary configuration to integrate the EESSI CernVM-FS (CVMFS) package repository into our system. By adding this new RPM repository definition, we are preparing the infrastructure to support EESSI CVMFS, which is crucial for managing scientific software environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds a new RPM package repository for the EESSI CVMFS configuration. However, the provided URL points to a single RPM file instead of a YUM repository, which is inconsistent with how other repositories are defined in this file and will likely cause the synchronization to fail. Additionally, the URL uses a 'latest' tag, which can lead to non-reproducible builds. My review includes a suggestion to use the correct YUM repository URL, pin it to a specific version for stability, and update related fields accordingly.
Updated EESSI CVMFS config URL to point to the new repository.
Co-authored-by: Alex Welsh <112560678+Alex-Welsh@users.noreply.github.com>
Alex-Welsh
left a comment
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 but I'd like to test it before merging. Can't do that right now with the cloudflare outage.
Alex-Welsh
left a comment
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.
Again, looks functionally correct to me.
Would be best to get sign off from @sjpb before merging though
sjpb
left a comment
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
No description provided.