-
Notifications
You must be signed in to change notification settings - Fork 38
feat: inline CAS backend support in client #246
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
Conversation
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>
if m.Hash != nil { | ||
displayValue = fmt.Sprintf("%s@%s", m.Value, m.Hash) | ||
name := m.Value | ||
if m.EmbeddedInline || m.UploadedToCAS { |
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 think this deserve a separate helper function like is_downloadable.
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.
yeah, the problem is that embedded doesn't mean that can be downloaded.
In fact artifact download doesn't work with embedded (inline) artifacts. We'd need to have an artifact-attestation mapping to make it work.
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 mean, we can call it differently but we already use this "if m.EmbeddedInline || m.UploadedToCAS" in two places.
Uploader: cc, | ||
Name: "OCI", | ||
// 100MB max size | ||
MaxSize: 100 * 1024 * 1024, |
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 would keep a separate limit for inline and a default one like this for CAS. I think I would lower this value
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.
Agreed, note that this is changing in #247
$ go run main.go --insecure cas-backend ls
┌──────────────────────────────────────┬─────────────────────────────────────┬──────────┬─────────────────────────────────────┬───────────────┬─────────┐ │ ID │ LOCATION │ PROVIDER │ DESCRIPTION │ LIMITS │ DEFAULT │
├──────────────────────────────────────┼─────────────────────────────────────┼──────────┼─────────────────────────────────────┼───────────────┼─────────┤ │ 36c9b3c2-6946-4972-a832-f0137d7646d7 │ us-central1-docker.pkg.dev/axiomati │ OCI │ fopoobar │ MaxSize: 100M │ false │
│ │ c-grove-366622/chainloop-demo-org │ │ │ │ │ ├──────────────────────────────────────┼─────────────────────────────────────┼──────────┼─────────────────────────────────────┼───────────────┼─────────┤
│ f99fbbae-2a7a-4be3-94bf-1850bfa4b8c2 │ │ INLINE │ Embed artifacts content in the atte │ MaxSize: 500K │ true │ │ │ │ │ station │ │ │
└──────────────────────────────────────┴─────────────────────────────────────┴──────────┴─────────────────────────────────────┴───────────────┴─────────┘
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.
re: reducing max size. Until we support changing it I'd not go lower than what we have today. We'd need to check the size of all our binaries and such to make sure they fit.
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.
Added a few comments but in general LGTM!
Adds support for inline CAS backend in the client.
This code basically allows passing a
CASBackend
object to the upload logic, this new struct contains thename
of the backend, the Max upload size and an optional uploader.If no uploader is provided, the logic will opt-in for a
inline
store of the content. Within the size validation constraints.This code does not change anything from the current behavior, next, we'll create a patch to enable it based on the settings in the backend.
Refs #201