-
Notifications
You must be signed in to change notification settings - Fork 128
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
Expose version of the registry service via API endpoint #860
Conversation
As discussed with @kkistm, we decided to contribute a patch addressing #21. I did not find any version indicator in the java server, so I introduced the version property and set it to the most recent tag. Please let me know if I overlooked something or whether we should use something other than the tag. |
name = "RegistryVersion", | ||
description = "Version of the registry service" | ||
) | ||
|
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.
Looks like an extra empty line.
registryVersion.version = properties.getProperty("version"); | ||
return registryVersion; | ||
} catch (IOException e) { | ||
throw new RuntimeException("Failed to load version from " + propertiesPath, e); |
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.
Other methods of the service throw ErrorResultException
, should it be the same here?
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.
If I understand the code correctly, ErrorResultException
seems to be used if the client side makes an argument error or uses an invalid token on a protected endpoint.
But of course you are right: If the version cannot be read, we should definitely log that so it shows in the server logs. And we should probably still return a value here, such as an 'unknown'
version string, instead of throwing a runtime exception. I will address this in a follow-up commit.
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 now used ErrorResultException
, as you suggested.
|
||
@GetMapping(path = "/api/version", produces = MediaType.APPLICATION_JSON_VALUE) | ||
public ResponseEntity<RegistryVersionJson> getServerVersion() { | ||
return ResponseEntity.ok(local.getRegistryVersion()); |
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 see other methods are catching the exceptions and then process it something like this:
...
} catch (ErrorResultException exc) {
return exc.toResponseEntity(RegistryEntityJson.class);
}
Should it be in the same way here?
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.
Right, this seems to be the pattern we should be following. I adjusted the method accordingly.
@@ -381,6 +381,26 @@ public String getPublicKey(String publicId) { | |||
} | |||
} | |||
|
|||
@Override |
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.
It doesn't look like the method will be used by any use-case. Should it be some stub instead?
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 am also unaware of a use case for now.
We could also reconsider extending the interface if there is no use case. I extended the IExtensionRegistry
interface because getRegistryVersion()
fulfills the interface declares "[...] registry API methods that can be accessed without authentication".
So we have three possibilities:
- Remove from interface
- Stub with UnsupportedOperationException
- Keep implementation even if not used for now
Which one would you prefer?
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.
For now I would propose to keep it, but add a comment that the functionality is not used at the moment, but could be used if there is a need to show a version of an upstream registry.
In future I would propose to revise the interfaces, maybe remove/add more methods.
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.
Good idea, done!
I squashed my commits into one, as suggested by @amvanbaren in #854. |
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.
@xai Can you add ovsx.registry.version
to the application.yml instead of using a properties file?
1aeaa6d
to
0b75caf
Compare
Done. I also rebased against master again due to an import conflict. |
ResponseEntity<RegistryVersionJson> response = restTemplate.getForEntity(urlTemplate, | ||
RegistryVersionJson.class); | ||
|
||
if (response.getStatusCode() == HttpStatus.OK && response.getBody() != null) { |
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.
You don't need to double check. Already handled by RestClientException
.
public ResponseEntity<RegistryVersionJson> getServerVersion() { | ||
try { | ||
return ResponseEntity.ok(local.getRegistryVersion()); |
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.
You can add a Cache-Control
header here, e.g. .cacheControl(CacheControl.maxAge(10, TimeUnit.MINUTES).cachePublic())
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.
Done
2a9f359
to
5ba3615
Compare
* server: The version number has to be updated in the application.yml file The endpoint /api/version can be queried to retrieve the version of the registry server. * webui: The version is displayed in the footer. Lazy/Suspend is used to not block rendering of the app with this call. Contributed on behalf of STMicroelectronics Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Server:
Web-ui:
Fixes #21