-
Notifications
You must be signed in to change notification settings - Fork 95
feat: langmgr and python #1091
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
base: main
Are you sure you want to change the base?
feat: langmgr and python #1091
Conversation
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
==========================================
+ Coverage 42.86% 45.40% +2.53%
==========================================
Files 35 37 +2
Lines 4122 4898 +776
==========================================
+ Hits 1767 2224 +457
- Misses 2224 2509 +285
- Partials 131 165 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
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.
just a few comments, otherwise LGTM!
pkg/langmgr/python.go
Outdated
var installCommands []string | ||
for _, pkgArg := range installPkgArgs { | ||
installCommands = append(installCommands, | ||
fmt.Sprintf(`pip install %s || echo "WARN: pip install failed for %s"`, pkgArg, pkgArg)) |
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.
should we do any validation/escaping here for the package names to prevent interpolation issues?
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.
good point, added validation functions to validate names and version
pkg/langmgr/python.go
Outdated
var uniqueFailedPkgsList []string | ||
if len(failedPackages) > 0 { | ||
for _, pkgName := range failedPackages { | ||
if !uniqueFailedPkgsMap[pkgName] { |
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.
it looks like this part might be duplicated a few times, perhaps we can pull out into a function like deduplicateStringSlice(input []string) []string
?
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.
added utils.DeduplicateStringSlice
pkg/langmgr/python.go
Outdated
} | ||
installCmd := fmt.Sprintf(`sh -c '%s'`, strings.Join(installCommands, "; ")) | ||
pipInstalled = pm.config.ImageState.Run( | ||
llb.Shlex(installCmd), |
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.
should we add a timeout for the pip install
command?
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.
by default pip has 15 seconds timeout. added a const for 300 seconds (5m), I am not sure if we want to make this configurable yet since we don't want too many knobs. maybe we can wait and see if someone wants a configurable timeout
|
||
// GetLanguageManagers returns a list of all available language managers. | ||
// As new language managers are implemented, they should be added to this list. | ||
func GetLanguageManagers(config *buildkit.Config, workingFolder string) []LangManager { |
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.
is this function only for testing?
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 is used in patch.go for getting the package managers (kinda equivalent to the GetPackageManager
for OS packages), but we only have python in this PR
|
||
#### `minor` Level | ||
|
||
- **Allows**: `2.6.0` → `2.6.1` (preferred) or `2.7.0` |
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 mean even if minor is set it will patch to 2.6.1 if both 2.6.1 and 2.7.0 are available?
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.
correct, since focus is on CVE fixes while maintaining compatibility as much as possible. It'll only bump to 2.7.0 if 2.6.1 doesn't fix that CVE
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
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.
Pull Request Overview
This PR introduces experimental support for app-level patching with Python package management in addition to existing OS package patching. The feature enables patching application-level dependencies using configurable patch levels and package type filtering.
Key changes:
- Adds experimental Python language manager for patching pip-managed packages
- Introduces
--pkg-types
flag to filter between OS and library packages - Implements
--library-patch-level
flag for controlling upgrade aggressiveness (patch/minor/major) - Updates manifest structure to separate OS and language updates
Reviewed Changes
Copilot reviewed 32 out of 33 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
pkg/langmgr/python.go | Core Python package manager implementation with pip integration |
pkg/patch/patch.go | Enhanced patching logic to support language managers and package filtering |
pkg/patch/cmd.go | New CLI flags for experimental package types and patch level control |
pkg/report/trivy.go | Extended Trivy parser with optimal version selection and library patch level support |
pkg/types/unversioned/types.go | Updated manifest structure separating OS and language updates |
website/docs/app-level-patching.md | Comprehensive documentation for the new feature |
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: Sertac Ozercan <[email protected]>
Signed-off-by: robert-cronin <[email protected]>
Signed-off-by: Sertac Ozercan [email protected]
Part of #147
Hold merge after current release
Describe the changes in this pull request using active verbs such as Add, Remove, Replace ...
Adds experimental app-level patching support with Python language manager
This PR introduces scaffolding for experimental support for patching application-level dependencies (libraries/packages) in addition to OS packages, with initial Python support.
Key Features
🔬 Experimental Feature
COPA_EXPERIMENTAL=1
environment variable📦 Package Type Filtering
--pkg-types
flag with values:os
,library
,os,library
os
(maintains backward compatibility)🎯 Patch Level Control
--library-patch-level
flag with values:patch
,minor
,major
patch
for conservative updates🐍 Python Support
pip
-managed Python packages. Other package managers are not considered for this initial PR.certifi
(always latest version)Example Usage