Make default and external name interchangeable for install and update commands for ORAS repos
#989
Conversation
isc-dchui
requested review from
isc-egabhart,
isc-eneil,
isc-jili,
isc-jlechtne,
isc-kiyer,
isc-pbarton and
isc-tleavitt
as code owners
isc-dchui
changed the title
install and update commands for ORAS repos
isc-kiyer
left a comment
isc-kiyer
left a comment
There was a problem hiding this comment.
@isc-dchui looks good! Few minor notes
| set msg = msg _ $char(10,13) _ "Response Code: "_response.StatusCode _ " - " _ response.Data.Read() | ||
| $$$ThrowStatus($$$ERROR($$$GeneralError, msg)) | ||
| } | ||
| // Use /v2/_catalog to enumerate all packages, then filter appropriately |
There was a problem hiding this comment.
My memory is very rusty on this, but I think that not all ORAS registries support /v2/_catalog and the intent here was:
- Support registries without /v2/_catalog if configured with a namespace
- Support registries registries with /v2/_catalog with/without a namespace.
This would seem to break the first case. It would be good if you could dig on this a tiny bit to confirm.
There was a problem hiding this comment.
@isc-tleavitt Ok after a bit of digging, you're correct that /v2/_catalog is an optional spec that not every registry supports. Unfortunately the OCI spec does not have a replacement. Individual registries may implement their own version, for example Docker Hub has https://hub.docker.com/v2/repositories/<namespace>/ but there is no standardization of what the endpoint is and what it returns. This does explain why searching in a namespace without a name has been broken: there is no registry-agnostic way of doing it without /v2/_catalog.
Not too sure what the correct approach is. I think we should use this endpoint if available, but then do we implement registry-specific handling if it is not?
There was a problem hiding this comment.
I've added some special handling to cover the case where /v2/_catalog is unavailable but name is specified. External name and default name won't be interchangeable in this case though
There was a problem hiding this comment.
@isc-dchui I don't want to do anything registry-specific, but if there is a generic way to work with a namespace and name, and without /v2/_catalog, I'd like to support it (as it seems we did prior to this change).
There was a problem hiding this comment.
Although I suppose as long as it works with zot and artifactory we're fine.
There was a problem hiding this comment.
Both zot and artifactory support the /v2/_catalog endpoint so we're good there at least
|
|
||
| #; Special case: if we provided an explicit build number, require it. | ||
| // Special case: if we provided an explicit build number, require it. | ||
| if ##class(%IPM.General.SemanticVersion).IsValid(searchCriteria.VersionExpression,.specificVersion) && (specificVersion.Build '= "") && (specificVersion.Build '= tVersion.Build) { |
There was a problem hiding this comment.
Sort of a side question since it's not in your review directly, but why don't we check whether the version expression in the search criteria is valid first thing in this method and throw an error if not?
There was a problem hiding this comment.
Good question! IsValid() checks whether it is a fully formed expression, e.g. 1.0.0, but the search criteria may include wildcards or other characters, e.g. 1.0.x., so we only want to make sure it is valid if it is a specific version
tests/integration_tests/Test/PM/Integration/PublishExternalName.cls
Outdated
Show resolved
Hide resolved
isc-jili
left a comment
isc-jili
left a comment
There was a problem hiding this comment.
@isc-dchui Looks really good! Thank you for helping implement this functionality. Left some small comments
| } | ||
|
|
||
| /// Given a string, tries to correlate it to an instance of <class>%IPM.Storage.Module</class>. | ||
| /// Throws errors. |
There was a problem hiding this comment.
I think it's great to comment explicitly on methods that throw errors. Would it be helpful for consistency to also comment on other methods on this PR that throw errors? For example ListModules() above throws errors but lacks this comment
There was a problem hiding this comment.
So the "Throws Errors" comment is only on the GetModuleObjectFrom methods and I just copied it over. Not too sure how I feel about it on all methods as often the code is pretty clear about throwing errors.
There was a problem hiding this comment.
I think if anything, its worth removing the throws errors comment. Only needed if its a specific error code we want to highlight
tests/integration_tests/Test/PM/Integration/PublishExternalName.cls
Outdated
Show resolved
Hide resolved
tests/integration_tests/Test/PM/Integration/PublishExternalName.cls
Outdated
Show resolved
Hide resolved
Resolves #959
Resolves #954
Resolves #789
Resolves #951
Note: the interchangeability of default and external name is only implemented for ORAS repos, as the module.xml is in the metadata. For remote repos, it would require pulling the entire module, which would add a significant overhead, so has been left unimplemented.