-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add option to filter image names or tags, parser fixes #159
Conversation
pkg/utils/config/config.go
Outdated
// ParseConfigFile parses a configuration file. | ||
func ParseConfigFile(configfile string) (*Config, error) { | ||
bfs := osfs.New(".") | ||
return ParseConfigFileFromFS(bfs, configfile) | ||
} | ||
|
||
func defaultConfig() *Config { | ||
return &Config{ | ||
Platform: "linux/amd64", |
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 rather default to no platform set?
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 should IMO
} | ||
return nil, fmt.Errorf("failed to decode config file: %w", err) |
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 change makes sure that an empty file is parsed and defaults used.
@@ -36,7 +38,7 @@ import ( | |||
const ( | |||
// ContainerImageRegex is regular expression pattern to match container image usage in YAML | |||
// nolint:lll | |||
ContainerImageRegex = `image\s*:\s*["']?([^\s"']+/[^\s"']+|[^\s"']+)(:[^\s"']+)?(@[^\s"']+)?["']?|FROM\s+([^\s]+(/[^\s]+)?(:[^\s]+)?(@[^\s]+)?)` | |||
ContainerImageRegex = `image\s*:\s*["']?([^\s"']+/[^\s"']+|[^\s"']+)(:[^\s"']+)?(@[^\s"']+)?["']?|FROM\s+(--platform=[^\s]+[^\s]*\s+)?([^\s]+(/[^\s]+)?(:[^\s]+)?(@[^\s]+)?)` |
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 ensures lines with --platform
even go through to the replacer.
pkg/replacer/image/image.go
Outdated
} | ||
|
||
fullRepositoryName := nameRef.Context().Name() | ||
parts := strings.Split(fullRepositoryName, "/") |
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 didn't find a better name to parse out the image name from the full repo path...
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.
Let's put this under a function, add a TODO item, and live with this for now. No biggie.
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.
done!
Adds two configuration options that let the user skip images that are either matching the configured image name or tag. Related: stacklok#150
- Fixes the default container regex to handle `FROM` directives that use a platform. Retains them when returning the modified reference - uses buildkit's dockerfile parser to get access to a real parser and use it to properly extract the image name and arguments such as `--platform` without brittle homecooked parsing - extends the image skipping to skip images or tags based on configuration - adds tests Fixes: stacklok#150 Fixes: stacklok#157
Adds a new configuration option that allows the user to set which branches should be excluded. By default, all are and thus frizbee only pins actions that are referred to with a tag. Fixes: stacklok#158
Let's reflect the recent changes in documentation
This will allow consumers to get the default configuration through an API and then customize only what they need.
} | ||
|
||
// TODO(jakub): this is a bit of a hack, but I didn't find a better way to get just the name | ||
func getImageNameFromRef(nameRef name.Reference) 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.
feel free to ignore this comment: What if we add unit tests specifically for this? That way, if someone refactors and comes up with a better way of doing it, it can be easy to spot problems or validate if the solution worked.
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 suggestion, I'll do it in a follow-up
FROM
directives that use a platform. Retains them when returning the modified reference. Uses buildkit's dockerfile parser to get access to a real parser and use it to properly extract the image name and arguments such as--platform
without brittle homecooked parsing. Extends the image skipping to skip images or tags based on configuration. Adds tests.