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

Show armeria version in doc service #685 #1744

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@KangWooJin
Copy link

commented Apr 25, 2019

Motivation:

  • The current armeria version of the doc service could not be showed in the armeria documentation service, so the version has been added to the title area.

Modification:

  • Add armeria version in DocService

Result:

  • Show armeria version in Armeria documentation service
@CLAassistant

This comment has been minimized.

Copy link

commented Apr 25, 2019

CLA assistant check
All committers have signed the CLA.

@KangWooJin KangWooJin force-pushed the KangWooJin:show-armeria-version-in-dos-service branch from c52e040 to 78a5460 Apr 25, 2019

@KangWooJin KangWooJin changed the title show armeria version in dos service #685 Show armeria version in dos service #685 Apr 25, 2019

@KangWooJin KangWooJin changed the title Show armeria version in dos service #685 Show armeria version in doc service #685 Apr 25, 2019

@KangWooJin KangWooJin force-pushed the KangWooJin:show-armeria-version-in-dos-service branch from 78a5460 to 2284890 Apr 25, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 25, 2019

Codecov Report

Merging #1744 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1744      +/-   ##
============================================
- Coverage     72.01%   71.94%   -0.07%     
+ Complexity     8215     8211       -4     
============================================
  Files           747      747              
  Lines         32959    32964       +5     
  Branches       4018     4018              
============================================
- Hits          23736    23717      -19     
- Misses         7183     7210      +27     
+ Partials       2040     2037       -3
Impacted Files Coverage Δ Complexity Δ
...java/com/linecorp/armeria/common/util/Version.java 66.12% <ø> (+8.06%) 12 <0> (+5) ⬆️
...a/com/linecorp/armeria/server/docs/DocService.java 94.93% <100%> (+0.16%) 37 <0> (ø) ⬇️
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-22.04%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-14.82%) 11% <0%> (-4%)
.../linecorp/armeria/server/tomcat/TomcatService.java 63.82% <0%> (-2.66%) 24% <0%> (-1%)
.../com/linecorp/armeria/server/docs/ServiceInfo.java 72.72% <0%> (-2.28%) 16% <0%> (-1%)
...spring/web/reactive/ArmeriaServerHttpResponse.java 75.24% <0%> (-1.99%) 28% <0%> (-1%)
...inecorp/armeria/client/grpc/ArmeriaClientCall.java 80.15% <0%> (-1.53%) 36% <0%> (-1%)
...common/stream/AbstractStreamMessageDuplicator.java 73.68% <0%> (-0.48%) 9% <0%> (ø)
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 71.77% <0%> (-0.48%) 48% <0%> (-1%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fd472d...3619925. Read the comment docs.

@trustin trustin added the new-feature label Apr 25, 2019

@@ -162,6 +165,10 @@ export class Specification {
return this.structsByName.get(name);
}

public getVersion(): string | undefined {

This comment has been minimized.

Copy link
@minwoox

minwoox Apr 26, 2019

Member

Is there any chance that the version is undefined?

This comment has been minimized.

Copy link
@KangWooJin

KangWooJin Apr 26, 2019

Author

I checked it again, and it doesn't happen.
I'll change it.

@@ -372,7 +372,7 @@ class App extends React.PureComponent<Props, State> {
color="inherit"
noWrap
>
Armeria documentation service
Armeria documentation service {specification.getVersion()}

This comment has been minimized.

Copy link
@minwoox

minwoox Apr 26, 2019

Member

I found out that we do not display the snapshot part when building site. Could you remove it as well, please? See https://github.com/line/armeria/blob/master/site/src/sphinx/conf.py#L37

This comment has been minimized.

Copy link
@trustin

trustin Apr 26, 2019

Member

I believe it's because we do not want to expose -SNAPSHOT in the official web site. I think a user should be given with full information in this case.

@@ -190,4 +196,12 @@ public ServiceSpecification(Iterable<ServiceInfo> services,
public List<HttpHeaders> exampleHttpHeaders() {
return exampleHttpHeaders;
}

/**
* Returns armeria version of the services in this specification.

This comment has been minimized.

Copy link
@minwoox

minwoox Apr 26, 2019

Member

the Armeria,
Returns the Armeria version of this specification.

This comment has been minimized.

Copy link
@KangWooJin

KangWooJin Apr 26, 2019

Author

Thank you for your recommendation.
I'll change it.

@minwoox

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Could you paste the rendered page so that people can easily see how it shows the Armeria version, please?

@trustin

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

I think the version information is not part of service specification. How about adding a separate child service and binding it at /versions.json, just like we did for /specification.json and /injected.js in the DocService constructor?

@trustin

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

How about exposing more information?

  • short commit hash
  • commit date
  • whether the repository was dirty (repositoryStatus)
@minwoox

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

binding it at /versions.json, just like we did for /specification.json and /injected.js in the DocService constructor?

Yeah, I thought it was OK to contain the version in the Specification, but probably not a good choice. We need a separate one for that. Thanks!

@trustin

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Gentle ping, @KangWooJin 😉

@KangWooJin

This comment has been minimized.

Copy link
Author

commented May 6, 2019

I'm sorry. I will develop this issue this week.

the version display will be displayed next to the title.

image

@KangWooJin

This comment has been minimized.

Copy link
Author

commented May 6, 2019

If you show me some additional information, what do you think it would be good to be exposed to the main page?

  • artifact version
  • short commit hash
  • commit date
  • whether the repository was dirty (repositoryStatus)

image

@KangWooJin KangWooJin force-pushed the KangWooJin:show-armeria-version-in-dos-service branch from 2284890 to 4f4bf14 May 6, 2019

Show armeria version in dos service #685
Fix checkstyle
Refactor version in docService
Remove print

@KangWooJin KangWooJin force-pushed the KangWooJin:show-armeria-version-in-dos-service branch from 4f4bf14 to 3619925 May 6, 2019

@trustin

This comment has been minimized.

Copy link
Member

commented May 8, 2019

the version display will be displayed next to the title.

Cool!

If you show me some additional information, what do you think it would be good to be exposed to the main page?

Sounds like a good idea!

@trustin
Copy link
Member

left a comment

IIUC you did not add the detailed version information to the main page, right?

@@ -161,8 +165,13 @@ public void serverStarting(Server server) throws Exception {
spec = addDocStrings(spec, services);
spec = addExamples(spec);

final Version version = Version.identify(DocService.class.getClassLoader())
.get(SERVER_ARTIFACT_ID);

This comment has been minimized.

Copy link
@trustin

trustin May 9, 2019

Member

Could we export the Version for all artifacts? i.e. you could just serialize the Map returned by identity().

@@ -110,6 +113,7 @@ public DocService() {
DocServiceFilter filter) {

super(ofExact("/specification.json", HttpFileService.forVfs(new DocServiceVfs())),
ofExact("/version.json", HttpFileService.forVfs(new DocServiceVfs())),

This comment has been minimized.

Copy link
@trustin

trustin May 9, 2019

Member

/versions.json

@@ -372,7 +378,7 @@ class App extends React.PureComponent<Props, State> {
color="inherit"
noWrap
>
Armeria documentation service
Armeria documentation service {version.getVersion()}

This comment has been minimized.

Copy link
@trustin

trustin May 9, 2019

Member

Then here, you could show the version of the artifact armeria.

@minwoox minwoox added this to the 0.86.0 milestone May 15, 2019

@hyangtack hyangtack removed this from the 0.86.0 milestone May 17, 2019

@minwoox minwoox added this to the 0.87.0 milestone May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.