Skip to content
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

Merged
merged 8 commits into from
Feb 6, 2025

Conversation

jfuerth
Copy link
Contributor

@jfuerth jfuerth commented Jan 17, 2025

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.

pkg/resource/check.go Show resolved Hide resolved
"github.com/Masterminds/semver/v3"
"github.com/pkg/errors"
"oras.land/oras-go/v2/registry"
"oras.land/oras-go/v2/registry/remote"
"slices"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"slices"

Copy link
Contributor

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

@kengou kengou requested review from IvoGoman and abhijith-darshan and removed request for auhlig January 17, 2025 21:10
Copy link
Contributor

@kengou kengou left a 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.

@kengou kengou linked an issue Jan 21, 2025 that may be closed by this pull request
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.
@jfuerth jfuerth requested review from a team as code owners January 27, 2025 19:32
This allows finding the latest semver tag in a repo that also has
non-semver tags such as deployed-in-prod or latest.
@kengou kengou changed the title Various improvements (fixes #12) feat(core): Various improvements (fixes #12) Jan 27, 2025
@kengou kengou changed the title feat(core): Various improvements (fixes #12) fix(ISSUE-12): various improvements Jan 27, 2025
@kengou kengou changed the title fix(ISSUE-12): various improvements fix(core): various improvements Jan 27, 2025
@jfuerth
Copy link
Contributor Author

jfuerth commented Jan 27, 2025

@jfuerth can you please rebase your pr based on master ? We did some changes to bring the repo up to speed.

@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.

@jfuerth
Copy link
Contributor Author

jfuerth commented Feb 3, 2025

@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.

@abhijith-darshan
Copy link

@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 action-semantic-pull-request action if possible.

Copy link
Contributor

@kengou kengou left a 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

pkg/resource/check.go Outdated Show resolved Hide resolved
@kengou
Copy link
Contributor

kengou commented Feb 4, 2025

@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 action-semantic-pull-request action if possible.

Only the PR title check is failing, will take a look into that later, that should not hinder us from merging

@jfuerth
Copy link
Contributor Author

jfuerth commented Feb 4, 2025

Looks good to me, only found one NIT. If you can take a look into that

Thanks for the suggestion. Applied!

@kengou kengou self-requested a review February 6, 2025 14:01
@kengou kengou merged commit 1482b86 into cloudoperators:main Feb 6, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various issues that prevented us from using this resource
4 participants