Page MenuHomePhabricator

Rename server to keyserver in relevant places in developement environment setup instructions
AbandonedPublic

Authored by marcin on Feb 28 2022, 6:22 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 9:51 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM
Unknown Object (File)
Fri, Nov 8, 8:16 PM

Details

Reviewers
tomek
atul
ashoat
Summary

Rename server to keyserver in dev_environment.md whenever it is mentioned as a file or a part of the application infrastructure.

Test Plan

No test plan since no runnable code is affected.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-390
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Retriggering CI for sanity check

tomek requested changes to this revision.Mar 1 2022, 8:56 AM
tomek added reviewers: atul, ashoat.

Some questions inline, but not sure if these are relevant. @atul please take a look. @ashoat I added you without accepting, just to make sure that I'm not suggesting something that is really off.

docs/dev_environment.md
12 ↗(On Diff #9981)

I'm not sure if this change is needed. We're mentioning here web and Android, which are general and not specific to our project, and we mix that with keyserver. I guess at that point it would be more appropriate to use general term server, because keyserver is a kind of server. But this is a decision which @ashoat needs to make.

102 ↗(On Diff #9981)

In this case it is fine to say server-side, but keyserver-side sounds strange. If we want to be specific here, we could say something like as the primary database on a keyserver.

610 ↗(On Diff #9981)

It's just a Node server, not a keyserver. Currently it's true that what is located in out keyserver directory serves the HTML, but I think we will change that, especially when server side rendering is gone, so we can as well update this then.

636 ↗(On Diff #9981)

Node will crash when it attempts to import those files

I think this is no longer the case, but updating that shouldn't be a part of this diff. The current behavior is that the server will run ok, so if you want e.g. a server and native, that would work.

766 ↗(On Diff #9981)

It's one word

This revision now requires changes to proceed.Mar 1 2022, 8:56 AM
docs/dev_environment.md
12 ↗(On Diff #9981)

Ok, then I will leave this unchanged and wait until @ashoat review.

636 ↗(On Diff #9981)

Should I then introduce any change regarding this part?

ashoat requested changes to this revision.Mar 11 2022, 6:37 AM

Agree with most of what @palys-swm said. By the way, something to note – if you request changes and add somebody to the review, the diff won't appear in their queue since it has "Needs Revision" status (and as such, appears in the diff author's queue)

docs/dev_environment.md
12 ↗(On Diff #9981)

I think it's fine to leave this as "keyserver"

102 ↗(On Diff #9981)

Agree that we should say server-side

610 ↗(On Diff #9981)

Agree we shoulds ay "Node server". (Note that this appears in a couple other places – please make the change throughout)

766 ↗(On Diff #9981)

Good catch

Differentiate between keyserver and server

Just a couple of Node keyserver that need to be replaced with Node server

docs/dev_environment.md
621 ↗(On Diff #10320)
720 ↗(On Diff #10320)
759 ↗(On Diff #10320)
636 ↗(On Diff #9981)

This change should be introduced later, as a new diff. We had an issue that tracked a lot of different docs improvements, so maybe we can add it there (or create a new one if that is not the case)

This looks great @marcinwasowicz! Agree with @palys-swm's final comments – thanks for maintaining consistency on "Node server" vs. "Node keyserver" there!

This revision is now accepted and ready to land.Mar 14 2022, 8:55 PM

Changes to consitently distuinguish between server and keyserver in dev env setup instructions.

Retrigger Ci after update