-
Notifications
You must be signed in to change notification settings - Fork 15
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
Security updates to containers #1336
base: devel
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR updates dependency versions, addresses a protobuf deprecation warning, and modifies the base image used for building containers. These changes aim to reduce image sizes, mitigate CVEs, and improve the overall security and compatibility of the DataFed system. Updated class diagram for protobuf message handlingclassDiagram
class Connection {
+recv(self, a_timeout=1000)
+makeMessage(self, msg_name)
}
note for Connection.recv "Uses MessageFactory to create message instances"
note for Connection.makeMessage "Uses MessageFactory to create message instances"
class MessageFactory {
+GetMessageClass(desc)
}
Connection -- MessageFactory : uses
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 @nedvedba - I've reviewed your changes - here's some feedback:
Overall Comments:
- It looks like you're updating dependencies - consider adding a comment to the changelog about these dependency updates.
- It looks like you're using a specific commit hash for the gcs submodule - is there a reason not to use a tagged release?
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.
DATAFED_NVM_VERSION="v0.39.7" | ||
DATAFED_NVM_VERSION="v0.40.1" | ||
# we cannot use node 22 even though it is the currently highest supported LTS version, due to a currently unsolved build error | ||
DATAFED_NODE_VERSION="v20.18.2" |
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.
Does this come with any breakages? I know @AronPerez was exploring an upgrade
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.
I have not noticed any, maybe @AronPerez can point out what he was running into?
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.
We can upgrade node, but we need to keep our web dependencies locked at their version
What I was running into was with node ~v16
: #1245
I have been pinged |
# RUN ${BUILD_DIR}/scripts/copy_dependency.sh boost_program_options to | ||
# RUN ${BUILD_DIR}/scripts/copy_dependency.sh boost_filesystem to |
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.
Can these be deleted?
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.
Anything that is commented out will be removed once I get the CI to pass.
PR Description
This PR serves to reduce image sizes and drastically reduce CVEs in built images, along with fixing a deprecation warning to the python API.
BEFORE
AFTER
Tasks
Summary by Sourcery
Updates dependencies, base images, and protobuf handling to reduce image sizes and CVEs, and fixes a deprecation warning in the Python API.
Bug Fixes:
Enhancements: