experimental doc service route guessing #4987

Dogacel · 2023-06-26T21:18:13Z

Motivation:

DocService gRPC set host name explicitly #4482

Modifications:

Add cfg.route() of the DocService to the ServiceSpecification as an optional value.

Result:

Example URL: http://127.0.0.1:9000/proxy/docs/#/methods/armeria.grpc.testing.TestService/UnaryCallWithAllDifferentParameterTypes/POST

Previously it sent requests to http://127.0.0.1:9000/test/armeria.grpc.testing.TestService/UnaryCallWithAllDifferentParameterTypes

but in reality it should be
URL: http://127.0.0.1:9000/proxy/test/armeria.grpc.testing.TestService/UnaryCallWithAllDifferentParameterTypes

Because DocService is bound under /docs not /proxy/docs.

To emulate, you can create the following proxy service:

    sb
        .virtualHost(9000)
        .serviceUnder("/proxy/", ProxyService())

// Under ProxyService

class ProxyService(val config: ProxyConfiguration) : HttpService {
    override fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse {
        val r2 = req.mapHeaders { headers -> headers.toBuilder().path(req.path().replace("/proxy", "")).build() }
        return client.execute(r2) // WebClient
    }
}

minwoox

This looks great. Thanks a lot! 😄

minwoox · 2023-06-27T10:55:19Z

core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java

@@ -149,6 +152,11 @@ public ServiceSpecification(Iterable<ServiceInfo> services,
        this.exampleHeaders = ImmutableList.copyOf(requireNonNull(exampleHeaders, "exampleHeaders"));
    }

+    public ServiceSpecification withServiceRoute(Route serviceRoute) {
+        this.serviceRoute = serviceRoute;


Global suggestion:
How about renaming serviceRoute to docServiceRoute which is more specific?

Would it make sense to set this route only when Route.pathType() is RoutePathType.PREFIX?

What about EXACT?

I am adding that as default too, seems like it shouldn't hurt.

We can't serve docserivce using EXACT so I think only one of PREFIX, REGEX and GLOB can be used.
If REGEX and GLOB are used, I thought we could just don't set it because we can't handle it. 😉

Makes sense, even if we can handle it seems like not worth it. Why would somebody mount DocService under a glob or regex path? 😅

About exact, this is related to how client code is generated, right? Because we are not serving a single static page but rather a page under path /docs/#/...?

Makes sense, even if we can handle it seems like not worth it. Why would somebody mount DocService under a glob or regex path?

That is correct. 😄

Because we are not serving a single static page but rather a page under path /docs/#/...?

That is also correct. The client sends additional requests whose path is:

/docs/main.js?somehash /docs/specification.json /docs/versions.json /docs/injected.js /docs/schemas.json /docs/assets/favicon.png

@minwoox Please take a second look when you are free 😄

Also, what is the standard procedure for testing those changes?

I am thinking about running Annotated, GraphQL, Grpc doc services and test everything is fine.

We don't have automated tests for UI. 😓
We definitely need to set up front-end tests.

core/src/test/java/com/linecorp/armeria/server/docs/ServiceSpecificationRouteTest.java

minwoox · 2023-06-30T07:01:33Z

docs-client/src/containers/MethodPage/DebugPage.tsx

@@ -203,6 +216,10 @@ const DebugPage: React.FunctionComponent<Props> = ({
        )?.pathMapping || '';
    }

+    if (urlPath.startsWith('/')) {


Question: Is there a case that urlPath doesn't start with a slash?

Hmm I was thinking about exact: and relative path (if possible?)

I think exact: will be stripped by the extractUrlPath method above but I'm not sure about the relative path.
So let's leave it as is. We can fix it later if we found any problems. 😉

trustin · 2023-07-05T08:16:53Z

core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java

+    /**
+     * Return a {@link ServiceSpecification} that contains the {@link Route} info of
+     * the attached {@link DocService}.
+     */
+    public ServiceSpecification withDocServiceRoute(Route docServiceRoute) {
+        this.docServiceRoute = docServiceRoute;
+        return this;
+    }


Can we keep this class immutable? It would also be nice if this method is not public.

Got it, I think it makes sense to move this route to exampleSupport because it contains path information as well? What do you think, I am not sure about creating a new class just to put a single method 😆

That's a good idea! We probably should rename it though. What do you think about SpecificationSupport?

Never mind. Just realized that you fixed it in a better way.

trustin · 2023-07-05T08:20:23Z

docs-client/src/containers/MethodPage/DebugPage.tsx

+const parseServerRootPath = (docServiceRoute: string) => {
+  const plainRoute = docServiceRoute.replace('/*', '');
+  const index = window.location.href.indexOf(plainRoute);
+
+  if (index === -1) {
+    return '';
+  }
+  const rootPath = window.location.href.substring(0, index);
+  return rootPath;
+};


Can we also show an error message if a user bound a DocService with other routes than prefix:? I think we can throw an exception on the server side, then we could simplify the logic here.

Good idea, exact paths don't work (I personally experienced it in the past).

Globs and regex, should we disable them?. I feel like it won't really make sense unless somebody has a particular need (i.e. bound it to a dynamic path).

I think so. Let's allow prefix: only.

jrhee17

Left some minor comments 👍
Can you also check if exact paths correctly use the base path?

i.e. if the following line is false:

armeria/docs-client/src/containers/MethodPage/DebugPage.tsx

Line 490 in 2e50636

if (!exactPathMapping) {

docs-client/src/lib/transports/transport.ts

jrhee17 · 2023-07-06T09:58:24Z

core/src/test/java/com/linecorp/armeria/server/docs/ServiceSpecificationRouteTest.java

+import com.linecorp.armeria.server.ServerBuilder;
+import com.linecorp.armeria.testing.junit5.server.ServerExtension;
+
+class ServiceSpecificationRouteTest {


What do you think of adding another service (maybe in AnnotatedDocServiceTest) so that we can at least verify manually easily if doc service works correctly behind a proxy?

e.g.

sb.serviceUnder("/proxy", (ctx, req) -> { final String origPath = req.path(); assertThat(origPath).startsWith("/proxy"); final String newPath = req.path().substring("/proxy".length()); final HttpRequest newReq = req.withHeaders(req.headers().toBuilder().path(newPath).build()); return server.webClient().execute(newReq); });

and we can call http://localhost:3000/proxy/docs when testing

jrhee17 · 2023-07-07T04:04:12Z

docs-client/src/containers/MethodPage/DebugPage.tsx

@@ -116,6 +117,17 @@ const copyTextToClipboard = (text: string) => {
  modal.removeChild(textArea);
 };

+const parseServerRootPath = (docServiceRoute: string) => {
+  const plainRoute = docServiceRoute.replace('/*', '');


What do you think of skipping unless the route ends with /*?
I'm thinking of regex cases where the path may look like: a/**/b

In backend we configured it in a way that it only works for PREFIX paths. If it is something else, docServiceRoute is going to be empty string. But it makes sense to just have the logic here as a safe-guard.

Updated logic to check route type.

jrhee17 · 2023-07-07T04:05:33Z

docs-client/src/containers/MethodPage/DebugPage.tsx

+    return '';
+  }
+  const rootPath = window.location.href.substring(0, index);
+  return rootPath;


Question) I haven't tried, but what do you think of returning a relative path instead of the full location? I feel like this can be a confusing point later if either an absolute path or a relative path can be set to additionalPath

i.e. /foo instead of http://host/foo

Hmm good idea, I was also thinking about the same thing let me try.

Now using a relative path. Tested manually behind a proxy and a non-proxy.

Dogacel · 2023-07-14T08:53:30Z

ditto,

I have a 3 day weekend, have some time to finalize my PRs.

Dogacel · 2023-07-18T17:10:58Z

Hey folks, I want to merge this if possible. We can't still use doc service for our staging services in my company so with this change I can promote usage of Doc service 🙂

jrhee17 · 2023-07-20T09:59:18Z

Hey folks, I want to merge this if possible. We can't still use doc service for our staging services in my company so with this change I can promote usage of Doc service 🙂

Thanks for the reminder. Will try to get review/get this merged in the next release (1.25.0).

core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java

jrhee17 · 2023-07-20T10:14:19Z

core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java

+     */
+    @JsonProperty
+    @Nullable
+    public Route docServiceRoute() {


I don't think we will always be aware that Route can be serialized to a json when modifying the class.

What do you think of either:

Transforming Route to a dedicated dto

Adding unit tests on json serialization which ensures that the contract won't change.

Yep, it is a good idea. Let me handle this.

OK, I found out that in ServiceSpecification we serialize the following structs to JSON as well:

HttpHeaders

MediaType

So I think it is OK as long as I add a custom serializer to it and add a unit test rather than creating a DTO. Because we did not create DTO for any of the above.

Hmm actually it is not really necessary to add a custom serializer either. How about just keeping @JsonProperty and adding unit tests?

How about just keeping @JsonProperty and adding unit tests?

Sure sounds good 👍

jrhee17 · 2023-07-20T10:20:19Z

core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java

+    @ParameterizedTest
+    @CsvSource({ "/proxy", "/proxy2/nested" })
+    void rightPathBehindProxy(String proxyPath) throws IOException {
+        server.server().reconfigure(sb -> sb.serviceUnder(proxyPath, (ctx, req) -> {


I prefer that we don't use reconfigure in tests since this can affect tests that might be added afterwards.
(Usually when adding tests to files like this, I think it is more convenient to check the added services in ServerExtension instead of having to check each test to see if the server has been configured)

I think it is more convenient to check the added services in ServerExtension

I did not understand this. My intention was to create the endpoint in test and test it afterwards. Kinda like scoping the server extension to single test 😆 So I wanted to make sure behavior is explicit. Any suggestions on how to do it? Maybe I should create different extensions or different test classes but that can be heavyweight.

Anyway, moved proxies to top-level as pre-defined endpoints for simplicity.

If a server.reconfigure() is called, the original services defined in ServerExtension are not restored.
Thus, if a test is executed after rightPathBehindProxy only /proxy2/nested and \docs service will be available.

Alternatively, we could enable ServerExtension#runForEachTest, but it's probably cheaper to just add the service like done in the following commit you added 👍 .

jrhee17

Looks good overall 👍
I think I'll have to run the service and check if there are any regressions, but basically I'm happy with the current PR if it wokrs 👍

jrhee17 · 2023-07-20T14:34:51Z

Were you able to check this comment? It seems like the actual request that is sent is not prefixed 😅

#4987 (review)

Dogacel · 2023-07-20T15:32:27Z

Sorry I must have missed that 😓

I'll get into it tomorrow 👍

jrhee17 · 2023-07-20T15:29:56Z

docs-client/src/lib/transports/thrift.ts

@@ -59,7 +59,7 @@ export default class ThriftTransport extends Transport {
      hdrs.set(name, value);
    }

-    return fetch(endpoint.pathMapping, {
+    return fetch(endpointPath || endpoint.pathMapping, {


I think most logic in DocService relies on using the endpoint's path, whereas we are adding a root path unrelated to endpoints.

Instead of reusing additionalPath (urlPath), what do you think of just passing the rootPrefix (docServiceRoute) all the way down to the transport layer?
i.e.

armeria/docs-client/src/lib/transports/annotated-http.ts

Lines 98 to 99 in a86bcc5

let newPath =

endpointPath || endpoint.pathMapping.substring('exact:'.length);

This way

We don't have to worry about edge cases depending on the method type.

We don't have to worry about url paths that are resolved at the last minute.

I did not understand. Do you think it would be better if we pass rootPrefix into this method,

armeria/docs-client/src/lib/transports/transport.ts

Lines 27 to 33 in 4bae026

public async send(

method: Method,

headers: { [name: string]: string },

bodyJson?: string,

endpointPath?: string,

queries?: string,

): Promise<string> {

And leave endpointPath unmodified? So doSend method takes rootPrefix instead?

I will push a PR regarding this and LMK if I understood this incorrectly.

BTW it seems like this || change in thrift should be fine regardless of that because same logic applies to annotated services?

I will push a PR regarding this and LMK if I understood this incorrectly.

On second thought, perhaps it's better to just add the prefix automatically to the preset UI. (so the original implementation was fine)
This way if the prefix adding behavior is not what the user wants, worst case the user can just manually remove the prefix.

I'll have to look into a better injection point for this behavior during the weekend, but if you have a better idea which solves this feel free to fix as you'd like. Sorry about the incorrect review 😅

No worries, I think your idea makes more sense. Prefix is similar to sending requests to another host (which we were discussing to implement anyway). If we want to show the URL we used to send the request, probably transport should return back the URL used to send because there are additional parsing happening in transport anyway. Should be better to centralize all those URL parsing. I had hard time modifying this code 😄

jrhee17 · 2023-07-20T15:34:02Z

docs-client/src/containers/MethodPage/DebugPage.tsx

@@ -203,6 +227,10 @@ const DebugPage: React.FunctionComponent<Props> = ({
        )?.pathMapping || '';
    }

+    if (urlPath.startsWith('/')) {
+      urlPath = parseServerRootPath(docServiceRoute) + urlPath;


Question) Should Copy as Curl also use the root path?

I think we should have the same behavior there as well, so yes.

Dogacel · 2023-07-26T07:08:16Z

Gentle ping @jrhee17 @minwoox

minwoox

Sorry for the late review. 😉
Looks good overall. 👍

minwoox · 2023-07-26T12:22:29Z

core/src/main/java/com/linecorp/armeria/server/docs/DocService.java

            return CompletableFuture.supplyAsync(() -> {
                final DocStringSupport docStringSupport = new DocStringSupport(services);
                ServiceSpecification spec = generate(services);
                spec = docStringSupport.addDocStrings(spec);
-                spec = exampleSupport.addExamples(spec);
+                spec = exampleSupport.addExamples(spec, docServiceRoute);


Technically, docServiceRoute isn't related to the example.
How about specifying it when calling generate method above?

ServiceSpecification spec = generate(services, docServiceRoute); // generate method private ServiceSpecification generate(List<ServiceConfig> services, Route docServiceRoute) { return ServiceSpecification.merge( plugins.stream() .map(plugin -> plugin.generateSpecification( findSupportedServices(plugin, services), filter, descriptiveTypeInfoProvider)) .collect(toImmutableList()), docServiceRoute); } // merge method public static ServiceSpecification merge(Iterable<ServiceSpecification> specs, Route docServiceRoute) { return new ServiceSpecification( Streams.stream(specs).flatMap(s -> s.services().stream())::iterator, Streams.stream(specs).flatMap(s -> s.enums().stream())::iterator, Streams.stream(specs).flatMap(s -> s.structs().stream())::iterator, Streams.stream(specs).flatMap(s -> s.exceptions().stream())::iterator, ImmutableList.of(), docServiceRoute); }

minwoox · 2023-07-26T12:24:12Z

core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java

+        if (docServiceRoute != null && docServiceRoute.pathType() == RoutePathType.PREFIX) {
+            return docServiceRoute;
+        }


How about doing this in the constructor?

if (docServiceRoute != null && docServiceRoute.pathType() == RoutePathType.PREFIX) { this.docServiceRoute = docServiceRoute; } else { this.docServiceRoute = null; }

minwoox · 2023-07-26T12:24:47Z

core/src/main/java/com/linecorp/armeria/server/docs/ServiceSpecification.java

@@ -121,6 +133,8 @@ private static void generateDescriptiveTypeInfos(
    private final Set<StructInfo> structs;
    private final Set<ExceptionInfo> exceptions;
    private final List<HttpHeaders> exampleHeaders;
+    @Nullable
+    private Route docServiceRoute;


nit:

Suggested change

private Route docServiceRoute;

private final Route docServiceRoute;

minwoox · 2023-07-26T12:31:47Z

core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServiceTest.java

+        private final String proxyPath;
+
+        private ProxyService(String proxyPath) {
+            super();


nit:

Suggested change

super();

minwoox · 2023-07-26T12:39:09Z

docs-client/src/containers/MethodPage/DebugPage.tsx

@@ -328,7 +353,7 @@ const DebugPage: React.FunctionComponent<Props> = ({
        `${window.location.port ? `:${window.location.port}` : ''}`;

      const httpMethod = method.httpMethod;
-      let uri;
+      let baseUri;


Suggested change

let baseUri;

let mappedPath;

because we use the term for this purpose? 😉

minwoox · 2023-07-26T12:42:56Z

docs-client/src/lib/transports/annotated-http.ts

@@ -86,6 +86,7 @@ export default class AnnotatedHttpTransport extends Transport {
    bodyJson?: string,
    endpointPath?: string,
    queries?: string,
+    pathPrefix?: string,


Can't this be pathPrefix because we specify an empty string when it's null?

Dogacel · 2023-08-11T04:25:30Z

@minwoox finally had some time to address the issues while waiting in the airport 😄 Let me know what you think, excited to have this feature in the next release 😄

jrhee17

Overall looks almost done 👍 It seems like the addExamples is omitting the route causing test failures.
Can you check it?

jrhee17 · 2023-08-14T15:30:52Z

docs-client/src/lib/transports/transport.ts

@@ -144,9 +146,9 @@ export default abstract class Transport {
   * Default implementation is suitable for RPC, using endpoint.pathMapping === path.
   */
  protected validatePath(endpoint: Endpoint, path: string): { error?: string } {
-    if (endpoint.pathMapping !== path) {
+    if (!path.endsWith(endpoint.pathMapping)) {


Question) Since we decided to pass the pathPrefix to the transport layer, I thought we can revert this change. Is there a code path which requires this change?

jrhee17 · 2023-08-14T15:36:57Z

docs-client/src/containers/MethodPage/index.tsx

@@ -239,6 +239,7 @@ const MethodPage: React.FunctionComponent<Props> = (props) => {
          useRequestBody={needsToUseRequestBody(props.match.params.httpMethod)}
          debugFormIsOpen={debugFormIsOpen}
          setDebugFormIsOpen={setDebugFormIsOpen}
+          docServiceRoute={props.specification.getDocServiceRoute()}


optional; I think also serverRoot can be passed for dedup, but I'm fine with the current approach.

jrhee17 · 2023-08-14T15:42:28Z

docs-client/src/containers/MethodPage/DebugPage.tsx

+
+  return window.location.pathname.substring(
+    0,
+    window.location.pathname.indexOf(docServicePath),


I think it's possible that docServicePath isn't even in the pathname.

i.e. a proxy server just rewrites the path (i.e. foo->bar) instead of removing a prefix.

What do you think of just defaulting to no root path in this case?

const index = window.location.pathname.indexOf(docServicePath); if (index < 0) { return ''; } return window.location.pathname.substring(0, index);

it should happen rarely and it makes sense to default to that in that case.

minwoox

This looks great to me. Thanks!

jrhee17

Looks good! Thanks @Dogacel 🙇 👍 🚀

docs-client/src/lib/transports/transport.ts

Co-authored-by: jrhee17 <guins_j@guins.org>

trustin

Excellent work and sorry for a late approval, @Dogacel. 🙇 We may want to disallow other route types than prefix, but it's probably something that we can take care of later.

codecov · 2023-08-17T09:14:02Z

Codecov Report

Patch coverage: 89.84% and project coverage change: +0.09% 🎉

Comparison is base (a73f78c) 74.22% compared to head (b6eca95) 74.31%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4987      +/-   ##
============================================
+ Coverage     74.22%   74.31%   +0.09%     
- Complexity    19518    19601      +83     
============================================
  Files          1674     1682       +8     
  Lines         71947    72260     +313     
  Branches       9217     9242      +25     
============================================
+ Hits          53400    53702     +302     
- Misses        14209    14217       +8     
- Partials       4338     4341       +3

Files Changed	Coverage Δ
...c/main/java/com/linecorp/armeria/server/Route.java	`66.66% <ø> (ø)`
...a/client/observation/ClientObservationContext.java	`63.63% <63.63%> (ø)`
.../server/observation/ServiceObservationContext.java	`63.63% <63.63%> (ø)`
...corp/armeria/server/docs/ServiceSpecification.java	`91.66% <72.72%> (-3.72%)`	⬇️
...servation/DefaultServiceObservationConvention.java	`89.23% <89.23%> (ø)`
...armeria/server/observation/ObservationService.java	`89.47% <89.47%> (ø)`
...bservation/HttpClientObservationDocumentation.java	`91.42% <91.42%> (ø)`
...servation/HttpServiceObservationDocumentation.java	`91.42% <91.42%> (ø)`
.../armeria/client/observation/ObservationClient.java	`91.89% <91.89%> (ø)`
...vation/DefaultHttpClientObservationConvention.java	`96.92% <96.92%> (ø)`
... and 4 more

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon

Thanks, @Dogacel!

Dogacel added 3 commits June 27, 2023 00:16

experimental doc service route guessing

4ceaf3a

fix actual proxy

cb0c7f4

fix thrift

2d5ca7c

Dogacel marked this pull request as ready for review June 26, 2023 23:57

Dogacel requested review from ikhoon, jrhee17, minwoox and trustin as code owners June 26, 2023 23:57

minwoox added the improvement label Jun 27, 2023

minwoox added this to the 1.25.0 milestone Jun 27, 2023

fix lint

250237c

minwoox reviewed Jun 30, 2023

View reviewed changes

rename servicePath to docServicePath

e2aa7cc

Dogacel requested a review from minwoox June 30, 2023 11:07

Dogacel added 3 commits June 30, 2023 15:03

fix tests 2

54bd813

fix tests by using relative path

a04ff08

fix thrift tests

09b9537

trustin reviewed Jul 5, 2023

View reviewed changes

jrhee17 reviewed Jul 7, 2023

View reviewed changes

Dogacel added 2 commits July 7, 2023 09:59

Backend changes for making service spec immutable

96c68e2

update frontend

f699ec3

Dogacel requested review from trustin and jrhee17 July 7, 2023 07:52

Dogacel added 2 commits July 7, 2023 11:15

fix tests and add new ones

3ce3e1a

fix lint + tests

fd4ef6a

Merge branch 'main' into dogac/experimental-figure-out-host-by-route

8466378

jrhee17 reviewed Jul 20, 2023

View reviewed changes

Dogacel requested a review from jrhee17 July 20, 2023 12:08

fix & add tests

4bae026

jrhee17 reviewed Jul 20, 2023

View reviewed changes

use path prefix logic

d99ee56

Dogacel force-pushed the dogac/experimental-figure-out-host-by-route branch from 44e491e to d99ee56 Compare July 21, 2023 08:11

minwoox reviewed Jul 26, 2023

View reviewed changes

Dogacel added 3 commits August 11, 2023 07:00

Merge branch 'main' into dogac/experimental-figure-out-host-by-route

cbdb8e1

fix compile err

16cf5d5

comment resolution

3460e49

Merge branch 'main' into dogac/experimental-figure-out-host-by-route

8d4267a

jrhee17 reviewed Aug 14, 2023

View reviewed changes

comment resolution and test fix

0da568d

minwoox approved these changes Aug 16, 2023

View reviewed changes

jrhee17 approved these changes Aug 16, 2023

View reviewed changes

docs-client/src/lib/transports/transport.ts Outdated Show resolved Hide resolved

Update docs-client/src/lib/transports/transport.ts

7af6fed

Co-authored-by: jrhee17 <guins_j@guins.org>

trustin approved these changes Aug 16, 2023

View reviewed changes

Merge branch 'main' into dogac/experimental-figure-out-host-by-route

b6eca95

ikhoon approved these changes Aug 18, 2023

View reviewed changes

ikhoon merged commit 20b4ca4 into line:main Aug 18, 2023
15 checks passed

	let newPath =
	endpointPath \|\| endpoint.pathMapping.substring('exact:'.length);

	public async send(
	method: Method,
	headers: { [name: string]: string },
	bodyJson?: string,
	endpointPath?: string,
	queries?: string,
	): Promise<string> {

	private Route docServiceRoute;
	private final Route docServiceRoute;

experimental doc service route guessing #4987

experimental doc service route guessing #4987

Conversation

Dogacel commented Jun 26, 2023 • edited

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dogacel commented Jul 14, 2023

Dogacel commented Jul 18, 2023

jrhee17 commented Jul 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 commented Jul 20, 2023

Dogacel commented Jul 20, 2023

jrhee17 Jul 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dogacel Jul 21, 2023 • edited

Choose a reason for hiding this comment

jrhee17 Jul 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dogacel commented Jul 26, 2023

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dogacel commented Aug 11, 2023

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Dogacel commented Jun 26, 2023 •

edited

jrhee17 left a comment •

edited

jrhee17 commented Jul 20, 2023 •

edited

jrhee17 Jul 20, 2023 •

edited

Dogacel Jul 21, 2023 •

edited

jrhee17 Jul 21, 2023 •

edited