-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix(core): various improvements #13
Conversation
pkg/resource/check.go
Outdated
"github.com/Masterminds/semver/v3" | ||
"github.com/pkg/errors" | ||
"oras.land/oras-go/v2/registry" | ||
"oras.land/oras-go/v2/registry/remote" | ||
"slices" |
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.
"slices" |
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 look at this file in detail. Everything else LGTM
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.
@jfuerth can you please rebase your pr based on master ? We did some changes to bring the repo up to speed.
Also fixed a warning about 'as' being lowercase.
When we parse a version string like `1.0-foo` which lacks the semver patch field, everything works fine in terms of sorting and filtering versions because the semver parser normalizes it to `1.0.0-foo`. But when we use the normalized version string to resolve the image digest, and propagate it as the `tag` in the resource, we end up with "Not Found" errors. This change ensures the original tag string is used for everything except sorting and comparing versions.
oras-go won't transmit credentials unless `registry` exactly matches the `host[:port]` of the HTTP request to the registry.
* now we return a list of versions starting from the requested one, up to the current one * if there is no requested version (eg. this is the first check for this resource) then we return the latest 10 versions.
f3a106f
to
e43e553
Compare
This allows finding the latest semver tag in a repo that also has non-semver tags such as deployed-in-prod or latest.
e43e553
to
8b1ae4b
Compare
@kengou done! I was busy with another project last week and it took longer than I would have liked to come back and rebase this on top of your changes. Note also: I've just tacked on one more small change based on our real-world experience using the resource: in check.go, we're now ignoring tags that aren't semver rather than failing the whole check on them. This arose because we were getting resource checking errors from a repo where someone had added a "deployed-in-prod" tag. |
@kengou @SuperSandro2000 just bumping this for your attention. The only failing check seems to be failing due to an image pull error. Maybe transient, but I don't have a retry button with my permissions. |
When workflows involve secret and a PR is made from a fork by integration design it would fail this is expected. @kengou maybe we can look into permissions for the |
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.
Looks good to me, only found one NIT. If you can take a look into that
Only the PR title check is failing, will take a look into that later, that should not hinder us from merging |
Co-authored-by: David Gogl <[email protected]>
Thanks for the suggestion. Applied! |
This is a handful of changes my team made to this resource in order to get things working for us. We think these changes will benefit others in the community.
See issue #12 for details.