ghcr.io: List packages based on whether owner is an org or a user #180
Conversation
We were previously using releases to figure out tags for a Github package. I think this was wrong. Not all release tags will be pushed as package versions, and vice versa. Someone may choose to use GHCR without taking advantage of releases at all. I've modified it so that the client will check if the owner is a user or an org and then make the appropriate PackageGetAllVersions function call to retrieve the tags. I've also fixed a few other things I ran into while testing this: 1. Create the github client in `New` so that rate limiting and other goodness is reused across calls. 2. Fix `RepoImageFromPath`, so it will split the repository path into the 'owner' and 'repo' segments that `Tags` expects. Previously we would have got errors for subrepositories. 3. Update `TestRepoImage` to ensure it doesn't panic on unexpected inputs. 4. We don't need to use regex to match `ghcr.io` to `ghcr.io`. 5. If we're excluding `.att` tags then we should probably exclude `.sig` and `.sbom` too.
pkg/client/ghcr/ghcr.go
Outdated
| // Choose the correct list packages function based on whether the owner | ||
| // is a user or an organization | ||
| getAllVersions := c.client.Organizations.PackageGetAllVersions | ||
| user, _, err := c.client.Users.Get(ctx, owner) |
There was a problem hiding this comment.
GitHub has a very aggressive rate limit - we should look to try and cache this somewhere/where possible as this could cause image discovery to be significantly delayed or fail.
There was a problem hiding this comment.
Addressed in efb6cc4.
|
|
||
| func getTagsFromRelease(client *github.Client, owner string, repo string) ([]api.ImageTag, error) { |
There was a problem hiding this comment.
Using releases, was a way of being significantly quicker than searching the repo due to GitHubs aggressive rate limiting... Maybe this could be an option/annotation perhaps to enable/disable or a fallback?
There was a problem hiding this comment.
Being able to say 'I want to use release versions to figure out latest' for specific repositories sounds like a good feature. We probably need to think about the best way to do that.
I would say that we could merge this PR, which fixes things for repositories that are currently broken, and then think about that in a follow up?
There was a problem hiding this comment.
I'm happy to merge if an Issue is raised to capture this? (Once I get time to re-review things 🙈 )
| func (c *Client) IsHost(host string) bool { | ||
| return reg.MatchString(host) | ||
| return host == "ghcr.io" | ||
| } |
There was a problem hiding this comment.
I get that this makes it simpler, but does make it harder or more complex if github decide to add additional subdomains etc or even their domain (not sure they would) additionally this does some what diverge the logic from other clients.
There was a problem hiding this comment.
I don't think the fact that we might need a regex down the line is a good reason to use a regex unnecessarily here. If GHCR starts using different domains, we could introduce a regex when that happens. We'd have to update the code either way in that situation.
I also don't think the implementation matching how things are done in other clients has much benefit either. As long as the interface is satisfied, I think the details of the implementation should be the simplest possible.
That being said, I just made this change in passing and it isn't really related to the PR, so happy to revert. It's more of a style thing than anything else.
We were previously using releases to figure out tags for a Github package. I think this was wrong. Not all release tags will be pushed as package versions, and vice versa. Someone may choose to use GHCR without taking advantage of releases at all.
I've modified it so that the client will check if the owner is a user or an org and then make the appropriate
PackageGetAllVersionsfunction call to retrieve the tags.I've also fixed a few other things I ran into while testing this:
Newso that rate limiting and other goodness is reused across calls.RepoImageFromPath, so it will split the repository path into the 'owner' and 'repo' segments thatTagsexpects. Previously we would have got errors for subrepositories.TestRepoImageto ensure it doesn't panic on unexpected inputs.ghcr.iotoghcr.io..atttags then we should probably exclude.sigand.sbomtoo.Fixes #179, #175