feat!: Use artifactory catalog - #377
Conversation
| return nil, fmt.Errorf("sending request: %w", err) | ||
| } | ||
|
|
||
| if response.StatusCode < http.StatusOK || response.StatusCode >= http.StatusMultipleChoices { |
There was a problem hiding this comment.
Why not != http.StatusOK?
There was a problem hiding this comment.
well, since we are trying to make it generic, I thought we might want to accept 204 No Content and 206 Partial Content, and the caller would handle such. Can stick to != http.StatusOK if you think that's better?
|
Is |
yejseo01
changed the title
| const ( | ||
| DefaultURL = "https://artifacts.tools.arm.com/devx-topo-project-catalog/" + version + "/catalog/" | ||
| DefaultCatalogURL = DefaultURL + "catalog.json" | ||
| DefaultSchemaURL = DefaultURL + "catalog.schema.json" |
There was a problem hiding this comment.
It feels a little unfortunate to download the schema for artifactory every time for every type of source. It means even users who use file:// sources require a network connection + slows the command down to download a static file every time.
Maybe fine for a v1 but wonder if we should consider downloading + embedding the schema version that the CLI supports into the binary? Especially since the catalogDocument type is a mirror of the schema.
Bonus - wonder if we should generate go types based on the schema?!
There was a problem hiding this comment.
Come to think of it, you’re right @awphi, this is a bit wasteful. I wonder if generating types based on the schema would be better. We could also do a pre-flight check of the $schema field in the received catalog - if it matches our expectation.
There was a problem hiding this comment.
yea I was thinking this on the way back home. For Bartek's suggestion, I'm not sure how that would alleviate user with file:// source from needing network connection? I think to address that, we have no choice but to keep the schema in the same repository
There was a problem hiding this comment.
Another thing we can try is to fetch the schema on release of topo, and embed that in the binary on release
ed774fc to
5b3f922
Compare
|
for error messages to precisely compare the versions, this PR needs to be merged first to continue - arm/topo-project-catalog#8 |
3 participants
Changes
Checklist