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?
There was a problem hiding this comment.
How would caller handle them right now?
There was a problem hiding this comment.
right now, they just check != http.StatusOK. Maybe you're right, and we won't need it. Will fix it to != http.StatusOK
|
Is |
yejseo01
changed the title
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 |
awphi
left a comment
awphi
left a comment
There was a problem hiding this comment.
Nice! Have approved arm/topo-project-catalog#8 so we can add the major version check.
awphi
left a comment
awphi
left a comment
There was a problem hiding this comment.
Nice! One implementation question and one error message nit!
awphi
left a comment
awphi
left a comment
There was a problem hiding this comment.
Couple more nits but nothing to block over (though please fix the variable casing!)
| return nil, fmt.Errorf("failed schema validation: %w", err) | ||
| catalogVersion, versionErr := unmarshalCatalogVersion(b) | ||
| catalogVersionMajor := majorVersion(catalogVersion) | ||
| SchemaVersionMajor := majorVersion(CatalogSchemaVersion) |
There was a problem hiding this comment.
| SchemaVersionMajor := majorVersion(CatalogSchemaVersion) | |
| schemaVersionMajor := majorVersion(CatalogSchemaVersion) |
Alternatively, could add a var majorCatalogVersion = majorVersion(CatalogSchemaVersion) at the top and re-use in the defaultURL to avoid repeating. This isn't a blocker though
| assert.ErrorContains(t, err, `requested catalog version "" is incompatible`) | ||
| }) | ||
|
|
||
| t.Run("reports catalog and schema versions when incompatible catalog fails to unmarshal", func(t *testing.T) { |
There was a problem hiding this comment.
| t.Run("reports catalog and schema versions when incompatible catalog fails to unmarshal", func(t *testing.T) { | |
| t.Run("reports incompatible catalog", func(t *testing.T) { |
Test name isn't overly verbose and wrong atm since it doesn't only report when unmarshaling fails
3 participants
Changes
Checklist