Skip to content

fix(core): various improvements#13

Merged
kengou merged 8 commits intocloudoperators:mainfrom
DNAstack:various-fixes
Feb 6, 2025
Merged

fix(core): various improvements#13
kengou merged 8 commits intocloudoperators:mainfrom
DNAstack:various-fixes

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.

"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
Collaborator

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

@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

@kengou
Copy link
Collaborator

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

Co-authored-by: David Gogl <1381862+kengou@users.noreply.github.com>
@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
@jfuerth jfuerth deleted the various-fixes branch July 4, 2025 20:45
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