Details

Reviewers

• karol
tomek
• jim
• max
ashoat

Commits

rCOMM265a154b4efd: [docs] Services documentation

Summary

Services building, configuration, and running documentation (GitHub).

Related linear task: ENG-669

Test Plan

No tests for markdown.

Diff Detail

Repository

rCOMM Comm

Branch

dev_services

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

• max added inline comments.Mar 2 2022, 12:50 PM

docs/dev_services.md
21 ↗	(On Diff #9852)	Maybe something like this? Looks good, changed.
22 ↗	(On Diff #9852)	We should either include the commands to run in a code block or link to specific docs? Could link to the specific section in the previously linked page that discusses the `aws configure` command? (https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html#cli-configure-files-methods) Makes sense. Added the paragraph about to run `aws configure`.
24 ↗	(On Diff #9852)	I think we can hyphenate this to "Tunnelbroker-specific" Agree, changed.
28 ↗	(On Diff #9852)	Maybe something like this? Correct me if I'm wrong, but I'm assuming that AMPQ is a stopgap solution and in the future we will exclusively use AMPQS? That's correct. And Amazon MQ support only secured AMQPS protocol.
30 ↗	(On Diff #9852)	I'm guessing there's an additional step to install the `rabbitmqctl` CLI? Is this something we should link to in this section? No, `rabbitmqctl` comes with the RabbitMQ setup/package. There are no additional steps to install it on the instance with runs RabbitMQ server.
37 ↗	(On Diff #9852)	Instead of wrapping these in double asterisk to make them bold, could we wrap them in backticks? `tunnelbroker.instance-id` instead of tunnelbroker.instance-id? Yep, maybe it will look better. Changed.
60–83 ↗	(On Diff #9852)	Instead of listing these here, which will require us to update this doc every time we modify `services/package.json`.. I think we can just point users to the file? That makes sense to me. I've removed this list and pointed to the file itself.

• jim added inline comments.Mar 2 2022, 1:11 PM

docs/dev_services.md
11 ↗	(On Diff #10035)	Also need to install docker-compose, which is a separate tool
17 ↗	(On Diff #10035)	What in services depends on `yarn`? I don't even really like that node/npm is a dependency just for the `npm run` scripts which are simple aliases. The scripts should just be documented so they can be run directly. Or one bash script to rule them all with subcommands.

Ok, there is something there, but you didn't even mention tests or the dev mode. Are you planning to do this?

docs/dev_services.md
8 ↗	(On Diff #10035)
17 ↗	(On Diff #10035)	We use the `yarn` package manager to install dependencies and run scripts This is not correct, we use it only for the scripts. @jimpo I think we talked about this already. We use npm scripts for other components of the app so it's good to go for consistency. Bash is bad, it's just is and if we have to use it, let's do it, but writing complex scripts in it should be avoided I believe. What's wrong with `yarn` that you keep bringing it up? Also, what's the decent alternative(let's give up on big bash scripts)?
30 ↗	(On Diff #10035)	What is this construction with the double dash? `--` Wouldn't it be better to use parenthesis? `(...)`

This revision now requires changes to proceed.Mar 3 2022, 3:22 AM

Update by comments.

Harbormaster completed remote builds in B7129: Diff 10040.Mar 3 2022, 4:10 AM

In D3240#89599, @karol-bisztyga wrote:

Ok, there is something there, but you didn't even mention tests or the dev mode. Are you planning to do this?

Good catch!
Updated with the short dev scripts paragraph in the npm section.
I think I should introduce the dev mode complete description as a follow-up diff after the dev mode for tunnelbroker will be introduced, so it would be for all the services and local instances description.
I've created the task for it blocked by the tunnelbroker dev-mode.

docs/dev_services.md
11 ↗	(On Diff #10035)	Also need to install docker-compose, which is a separate tool Oh, right. Updated. Thanks.
17 ↗	(On Diff #10035)	What in services depends on `yarn`? I don't even really like that node/npm is a dependency just for the `npm run` scripts which are simple aliases. The scripts should just be documented so they can be run directly. Or one bash script to rule them all with subcommands. I don't think using one bash script instead of the `npm` is a good idea. We are using the `npm` as a "single point of run". In this case, I think we should stay with this approach and agree with @karol-bisztyga here.
30 ↗	(On Diff #10035)	What is this construction with the double dash? `--` Wouldn't it be better to use parenthesis? `(...)` Oh, I think we should remove it here.

• jim added inline comments.Mar 3 2022, 7:41 AM

docs/dev_services.md
17 ↗	(On Diff #10035)	For this diff I'm just saying that `yarn` is not a dependency, `npm` is. Yes, you could use either but `npm` is bundled with node so I wouldn't call yarn a dependency. "We use the Node package manager `npm` (or `yarn` if preferred) to run scripts." We can continue the discussion on whether this choice makes sense on chat or Linear.

Thanks for addressing feedback!

Accepting to unblock since my issues were resolved, not because I think it's ready to land. But, I think it'll be ready to land once @karol-bisztyga's feedback is addressed and he accepts.

(Not sure if I'm doing the right thing here? I don't want to be blocking this diff, but I also don't want to indicate that it I think it's immediately ready to land... maybe I should resign instead of accepting?)

(Not sure if I'm doing the right thing here? I don't want to be blocking this diff, but I also don't want to indicate that it I think it's immediately ready to land... maybe I should resign instead of accepting?)

I think what you did makes sense.

Since @karol-bisztyga requested changes at some point, the diff won't have the "Accepted" status until he accepts it. By requesting changes, @karol-bisztyga basically made himself a blocking reviewer.
If @karol-bisztyga hadn't previously requested changes, but you wanted to make it so the diff doesn't have "Accepted" status until he accepts it, then you could set him as a blocking reviewer.
The distinction between resigning and accepting should pertain just to your individual judgement, and shouldn't have any relation to which reviewer are blocking the diff. I view resign as "I don't have context here"... it's not appropriate in this case because you have already reviewed the diff.

Thanks @geekbrother for creating a task for this!

LGTM

This revision is now accepted and ready to land.Mar 4 2022, 3:06 AM

Herald added a subscriber: • benschac. · View Herald TranscriptMar 4 2022, 3:06 AM

• jim mentioned this in D3237: tunnelbroker: Use pkg-config to find dependencies.Mar 4 2022, 12:52 PM

The style here is not consistent with our existing style. One clear example is that there are lines separated by a single line break, which is something we never do in dev_environment.md.

When putting up a docs diff or reviewing one, it is critical that you think of yourself as the editor. It is YOUR responsibility to maintain the consistency of style.

For every single line you write, you should ask yourself: how are similar lines written in our existing docs?

In this case, we still have a lot of inconsistency in style. I could do yet another line-by-line review, but I really want to get away from this state of the world, where the only way for us as a team to maintain consistency in docs is for the CEO to do a line-by-line review. This is simply not sustainable for the team.

I've chatted with @atul and @varun and we're going to try moving away from @atul handling reviews of docs diffs to @varun handling it. @varun, can you take a look here?

This revision now requires changes to proceed.Mar 7 2022, 12:03 PM

• max planned changes to this revision.Mar 10 2022, 11:28 AM

• max marked an inline comment as done.

Fixing the structure and newlines. VSCode paragraph was added.

In D3240#90478, @ashoat wrote:

The style here is not consistent with our existing style. One clear example is that there are lines separated by a single line break, which is something we never do in dev_environment.md.

When putting up a docs diff or reviewing one, it is critical that you think of yourself as the editor. It is YOUR responsibility to maintain the consistency of style.

For every single line you write, you should ask yourself: how are similar lines written in our existing docs?

In this case, we still have a lot of inconsistency in style. I could do yet another line-by-line review, but I really want to get away from this state of the world, where the only way for us as a team to maintain consistency in docs is for the CEO to do a line-by-line review. This is simply not sustainable for the team.

I've chatted with @atul and @varun and we're going to try moving away from @atul handling reviews of docs diffs to @varun handling it. @varun, can you take a look here?

Agree with your comments here. Must be careful and make it consistent with the existing docs.
@varun Can you take a look?

Harbormaster completed remote builds in B7442: Diff 10475.Mar 17 2022, 3:27 AM

Removing myself from the review so @varun can take a first pass. @varun, when you accept please add me back as a blocking reviewer!

docs/dev_services.md
39	Seems a bit weird that we're using an `inline code` Markdown element effectively as a subheading

varun requested changes to this revision.Mar 18 2022, 2:10 PM

varun added inline comments.

docs/dev_services.md
3
5	Do we really need to mention that the base image is Ubuntu?
15
19
23–25
27	Is the last sentence about AMQPS really needed?
39	agree, we should use triple backticks here
39–55	the contents of the .ini file should be in a single fenced code block. you can use inline comments in the code block to explain what the config values are. look at this example for how to properly format this section: https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#urls
59	Can you explain what dev-mode does?

This revision now requires changes to proceed.Mar 18 2022, 2:10 PM

• max planned changes to this revision.Mar 24 2022, 7:18 AM

Updates on comments.

docs/dev_services.md
5	Do we really need to mention that the base image is Ubuntu? I think we need to mention this because it's a significant difference between some of the Linux distros.
27	Is the last sentence about AMQPS really needed? I think we should keep this because only AMQPS is supported at the moment.
39	Seems a bit weird that we're using an `inline code` Markdown element effectively as a subheading agree, that makes sense to me too.
39	agree, we should use triple backticks here Yep, that would look nicer.
39–55	the contents of the .ini file should be in a single fenced code block. you can use inline comments in the code block to explain what the config values are. look at this example for how to properly format this section: https://github.com/CommE2E/comm/blob/master/docs/dev_environment.md#urls Changed it to be more descriptive and the same as we already have in `dev_environment.md#urls`.
59	Can you explain what dev-mode does? Yes, I've added a few words about it. As the dev-mode is not fully developed, I've added only a few.

Harbormaster completed remote builds in B7593: Diff 10664.Mar 24 2022, 8:24 AM

varun commandeered this revision.Mar 29 2022, 7:46 AM

varun edited reviewers, added: • max; removed: varun.

This revision is now accepted and ready to land.Mar 29 2022, 7:46 AM

A couple small edits

Made a couple small edits. This is really close -- just one inline question for @geekbrother before we put this on ashoat's queue

docs/dev_services.md
53 ↗	(On Diff #10772)	What happens if a developer connects to the prod instances of S3 and DynamoDB? Do we have any way to prevent that and should we explicitly direct devs to use the dev-mode scripts here?

Harbormaster completed remote builds in B7676: Diff 10772.Mar 29 2022, 7:55 AM

varun marked an inline comment as not done.Mar 29 2022, 8:04 AM

atul removed a reviewer: atul.Mar 30 2022, 6:42 PM

Making sure @geekbrother sees this by setting him to a blocking reviewer

I think it looks good for now. We will add more about the "dev" environment in the follow-up diffs.

docs/dev_services.md
53 ↗	(On Diff #10772)	What happens if a developer connects to the prod instances of S3 and DynamoDB? Do we have any way to prevent that and should we explicitly direct devs to use the dev-mode scripts here? I don't think we need to prevent this somehow because to connect to prod instances the developer must fulfill production credentials in the config file. So he can't just clone and connect to the prod. The good idea is to show somehow the mode while starting the service.