Skip to content
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

Should property "ToolVersion.containerfile" and other flags be required or dropped? #154

Open
uniqueg opened this issue Sep 19, 2020 · 3 comments

Comments

@uniqueg
Copy link
Collaborator

uniqueg commented Sep 19, 2020

I could imagine that this property was perhaps introduced to save a client one or multiple (if different types are provided for the same version) calls to GET /tools/{id}/versions/{version_id}/{type}/containerfile only to find out that there isn't a containerfile available for the tool. But if that is the case, wouldn't it be useful to make this property required in the response? I suppose it would be easy enough for a TRS implementation to auto-set this flag based on whether a containerfile was included with that version.

Incidentally, I kinda feel the same way about the other flags (Tool.has_checker, ToolVersion.is_production, ToolVersion.verified and ToolVersion.signed). If a client cannot rely on them to be set, it requires it to implement a way to get that information by inspecting a tool (version) for cases where the flags are not set, ideally in addition to implementing "fast track" checks that might save some time/calls if the flags are set. Alternatively, I would probably rather drop those flags altogether, as this at least reduces the risk of ambiguity in cases where services fail to properly validate the tool (version) data upon registration (e.g., containerfile is false, but a containerfile is in fact present).

┆Issue is synchronized with this Jira Story
┆containerName: GA4GH tool-registry-service
┆Issue Number: TRS-41

@denis-yuen
Copy link
Member

I think I agree for containerfile (i.e. I would be fine with making it required).

For has_checker, is_production, verified, and signed. I would note that I don't think one can determine these by inspecting the tool descriptors. For us (Dockstore), the library we're using (Java version of swagger-codegen) probably assumes that a missing property is understood to be false. But I'm ok with making them required as well.

@uniqueg
Copy link
Collaborator Author

uniqueg commented Sep 22, 2020

Agreed that signed (btw, not sure what that means exactly, the description is not very informative) cannot be inferred by the implementation. But for has_checker and verified there are also checker_url and verified_source, respectively, which could possibly be used by an implementation to set the values of the flags. Or at least that's my reading. Why supply a verification source if the tool wasn't actually verified? Or how useful is a has_checker=true if there isn't actually a checker_url available?

I think at the very least these properties and their dependencies on each other would benefit from being more clearly documented in their descriptions. When implementing POST and PUT endpoints for tools a couple of days/weeks ago in our TRS implementation, we first chose to omit them from the schemas and infer them from the other properties as described above, but then we pulled back and added them, since we felt we didn't really understand them and thus couldn't be sure that there aren't, in fact, valid use cases for setting them independently (and possibly ambiguously).

@denis-yuen
Copy link
Member

Oh, has_checker and verified probably are redundant with checker_url and verified_source

I just meant that they're different from containerfile which can be determined by inspecting the descriptors. By contrast, these properties shouldn't be ambiguous in the context of "risk of ambiguity in cases where services fail to properly validate the tool"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants