-
Notifications
You must be signed in to change notification settings - Fork 54
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
scan: ability to override repository #1040
base: main
Are you sure you want to change the base?
Conversation
Dentrax
commented
Jul 8, 2024
cbd5210
to
0218df1
Compare
pkg/cli/scan.go
Outdated
// getRepositoryURL returns the URL of the APKINDEX.tar.gz file for the given | ||
// repository and architecture. If the repository URL already points to an | ||
// APKINDEX.tar.gz file, it will be returned as-is. User input may or may not | ||
// have included the architecture or the APKINDEX.tar.gz suffix, so construct | ||
// the full URL to provide better UX. | ||
func getRepositoryURL(repository, arch 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.
Would it be possible to make the behavior simpler and more predictable, by requiring the caller to pass the URL to the repo and not to the index tar gz?
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.
Could you provide some example on this?
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 wondering if we need to support both with and without the .../APKINDEX.tar.gz
... It'd be simpler to say the URL has to be just to the repo, so like https://packages.wolfi.dev/os
, instead of accepting multiple forms, unless we really need to support both?
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.
if we need to support both
IIUC, apk ls
command supports both, and some packages does provide only single architecture, thats where we may need to pass the ARCH
in the URL.
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.
some packages does provide only single architecture
If I'm following you, this problem exists with the remote scanning feature with or without this new enhancement, is that right?
Would a better solution here be to show a warning if not all architectures are found? And still error if none can be found?
I guess I'm not following how architecture availability is specific to this new flag, but maybe you can help me follow :)
bae53cb
to
594a172
Compare
Signed-off-by: Dentrax <[email protected]>
594a172
to
6c99b9e
Compare