Page MenuHomePhabricator

[services] Add dev-mode to the services documentation
ClosedPublic

Authored by max on Jun 13 2022, 7:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM
Unknown Object (File)
Sun, Nov 3, 3:40 PM

Details

Summary

This diff introduces the services documentation section for the dev-mode for services.
Describe how to use the localstack and rabbitMQ cloud for development and testing purposes.

Linear task: ENG-823

Test Plan

Running commands in the documentation work as expected according to the doc.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
docs/dev_services.md
65 ↗(On Diff #13475)

localstack -> LocalStack

Fixed.

65–66 ↗(On Diff #13475)

There should be an additional line break between these two paragraphs.

Fixed.

67 ↗(On Diff #13475)

This shouldn't be on its own line

Fixed.

75 ↗(On Diff #13475)

hmm does it really start rabbitmq?

Hmm, nice catch it starts only the localstack and initializes the terraform. I've changed this.

80 ↗(On Diff #13475)

may not be super-clear what is what here. Maybe we could go with something like I suggested?

Why not, done.

83 ↗(On Diff #13475)

grammarly

Fixed.

96 ↗(On Diff #13475)

new line

Fixed.

tomek requested changes to this revision.Jun 21 2022, 1:10 AM
tomek added inline comments.
docs/dev_services.md
65 ↗(On Diff #13608)

I think this will look strange when formatted It uses LocalStack local cloud including.... Maybe we can use a different link text?

72 ↗(On Diff #13608)

The command is outdated D4303

83 ↗(On Diff #13608)

We capitalize tunnelbroker in other places

86 ↗(On Diff #13608)

This is inconsistent with the description. We say that a command looks like this yarn run-[service-name]-dev-mode, and the name is tunnelbroker, so the command should be yarn run-tunnelbroker-dev-mode.

If this command is correct, we should update the description to use yarn run-[service-name]-service-dev-mode. We haven't specified what we mean by service-name, but it's fair to assume that -service is not a part of it. If we expect -service to be a part of service-name we should clearly tell it somewhere.

This revision now requires changes to proceed.Jun 21 2022, 1:10 AM
max marked 4 inline comments as done.

Updating based on the @palys-swm comments.

docs/dev_services.md
65 ↗(On Diff #13608)

I think this will look strange when formatted It uses LocalStack local cloud including.... Maybe we can use a different link text?

Yes, I think we should use this text.

72 ↗(On Diff #13608)

The command is outdated D4303

Updated this diff based on the D4303 changes.

83 ↗(On Diff #13608)

We capitalize tunnelbroker in other places

Thanks for catching it!

86 ↗(On Diff #13608)

This is inconsistent with the description. We say that a command looks like this yarn run-[service-name]-dev-mode, and the name is tunnelbroker, so the command should be yarn run-tunnelbroker-dev-mode.

If this command is correct, we should update the description to use yarn run-[service-name]-service-dev-mode. We haven't specified what we mean by service-name, but it's fair to assume that -service is not a part of it. If we expect -service to be a part of service-name we should clearly tell it somewhere.

I've changed the command template to the yarn run-[service-name]-service-dev-mode to be consistent. Thanks!

cc to @palys-swm and @varun to re-review it.

varun requested changes to this revision.Jun 21 2022, 10:49 AM
varun added inline comments.
docs/dev_services.md
65 ↗(On Diff #13631)
67 ↗(On Diff #13631)

Not sure I understand what this means

67 ↗(On Diff #13631)

"a [RabbitMQ]"

75 ↗(On Diff #13631)

Why are we hyperlinking to this website twice?

83 ↗(On Diff #13631)

"for Tunnelbroker"

95 ↗(On Diff #13631)

no need to keep linking to this website

95 ↗(On Diff #13631)

I don't think this sentence is necessary

This revision now requires changes to proceed.Jun 21 2022, 10:49 AM
max marked 7 inline comments as done.

Updating based on @varun comments.

docs/dev_services.md
67 ↗(On Diff #13631)

Not sure I understand what this means

Slightly changed this sentence. Maybe now it's clearer.

67 ↗(On Diff #13631)

"a [RabbitMQ]"

Fixed.

75 ↗(On Diff #13631)

Why are we hyperlinking to this website twice?

It's a common practice in the docs to hyperlink all the occurrences, but I don't mind making it only once. Let's go with the only hyperlink. Changed this.

83 ↗(On Diff #13631)

"for Tunnelbroker"

Fixed.

95 ↗(On Diff #13631)

I don't think this sentence is necessary

Removed it.

95 ↗(On Diff #13631)

no need to keep linking to this website

Removed the whole sentence.

tomek added inline comments.
docs/dev_services.md
67 ↗(On Diff #13638)
69 ↗(On Diff #13638)

Normally I would request changes, but seeing as I'm going to be away through Sunday, I'm accepting here so we don't block @geekbrother from reaching his deadline.

@geekbrother & reviewers – please make sure that:

  1. My feedback is addressed before this gets landed
  2. Any new language is examined under a magnifying glass!
docs/dev_services.md
63 ↗(On Diff #13638)
  1. "dev-mode" doesn't sound clean... it feels like we're using unnecessary jargon here. Why not just "dev mode"? That's how we usually refer to it in text, I think
  2. It seems like we shouldn't even be calling this "dev mode".... how about just "Running services locally"?
65 ↗(On Diff #13638)

Does this make it easier, or is it basically required? Is there another supported way to run tests, for instance?

My impression is that this is the default way we're going to be using for integration tests going forward, so I think we should not frame this install as optional.

75 ↗(On Diff #13638)

Docker should be capitalized. Please be thoughtful about capitalization... it's a waste of everybody's time when people have to repeatedly give you feedback on styling names in docs diffs

77–93 ↗(On Diff #13638)
  1. I think this should be in its own section, eg. ## Running in dev mode
  2. The section should start with a simple explanation of what dev mode is / what dev mode does, and why you might want to run services in dev mode
max marked 6 inline comments as done.

Updates based on comments.

Normally I would request changes, but seeing as I'm going to be away through Sunday, I'm accepting here so we don't block @geekbrother from reaching his deadline.

@geekbrother & reviewers – please make sure that:

  1. My feedback is addressed before this gets landed
  2. Any new language is examined under a magnifying glass!

Sure! Thank you for your comments and for accepting this.

docs/dev_services.md
63 ↗(On Diff #13638)
  1. "dev-mode" doesn't sound clean... it feels like we're using unnecessary jargon here. Why not just "dev mode"? That's how we usually refer to it in text, I think
  2. It seems like we shouldn't even be calling this "dev mode".... how about just "Running services locally"?

I agree that "Running services locally" would be more informative and clearer. Changed it.

65 ↗(On Diff #13638)

Does this make it easier, or is it basically required? Is there another supported way to run tests, for instance?

My impression is that this is the default way we're going to be using for integration tests going forward, so I think we should not frame this install as optional.

I Agree that it can be misleading. I'll change this to be required actually.

69 ↗(On Diff #13638)

Grammarly shows an issue when we convert the sentence to such way. I think it's more natural for English to use the original sentence. Passing to @varun to check about this.

75 ↗(On Diff #13638)

Docker should be capitalized. Please be thoughtful about capitalization... it's a waste of everybody's time when people have to repeatedly give you feedback on styling names in docs diffs

I'm sorry, missed this one. Fixed and re-checked all such occurrences.

77–93 ↗(On Diff #13638)
  1. I think this should be in its own section, eg. ## Running in dev mode
  2. The section should start with a simple explanation of what dev mode is / what dev mode does, and why you might want to run services in dev mode

That makes sense to me. I've added the section with the complete description of this mode.

Passing it to @varun to make a final review.

varun requested changes to this revision.Jun 26 2022, 9:37 PM

Sorry this is going through so many iterations.... @geekbrother, let's get on a call today or tomorrow and pair on this to get it landed

docs/dev_services.md
67 ↗(On Diff #13722)

This paragraph doesn't really flow from the previous. I can commandeer this diff if this feedback isn't clear

77 ↗(On Diff #13722)

It's not clear to me what the difference is between "running services locally" and "running services in dev mode"

This revision now requires changes to proceed.Jun 26 2022, 9:37 PM
max marked 2 inline comments as done.

Slightly restructured text.

In D4253#123047, @varun wrote:

Sorry this is going through so many iterations.... @geekbrother, let's get on a call today or tomorrow and pair on this to get it landed

Sure! We can do it today. Thanks, @varun

docs/dev_services.md
67 ↗(On Diff #13722)

This paragraph doesn't really flow from the previous. I can commandeer this diff if this feedback isn't clear

I've slightly restructured the doc updates.
Moved this paragraph slightly upward. Maybe now it's more clear.

77 ↗(On Diff #13722)

It's not clear to me what the difference is between "running services locally" and "running services in dev mode"

I've changed the paragraph titles and slightly restructured them. Maybe now it's clearer.

varun edited reviewers, added: max; removed: varun.
This revision is now accepted and ready to land.Jun 27 2022, 1:23 PM

Replace "dev mode" terminology with "sandbox." This hopefully drives home the distinction between running one-off services locally with docker compose and running them in a local cloud environment.

varun foisted this revision upon max.
varun edited reviewers, added: varun; removed: max.

follow-up task for @geekbrother : change the yarn commands to something like yarn run-all-services-in-sandbox and yarn run-tunnelbroker-service-in-sandbox

In D4253#123390, @varun wrote:

follow-up task for @geekbrother : change the yarn commands to something like yarn run-all-services-in-sandbox and yarn run-tunnelbroker-service-in-sandbox

Thanks, @varun ! ENG-1299 is created as a follow-up task.