Page MenuHomePhabricator

Refresh the deployment documentation
ClosedPublic

Authored by nate on Dec 10 2019, 11:08 PM.

Details

Summary

took a cut at refreshing the deployment docs!

Test Plan

docs only

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nate created this revision.Dec 10 2019, 11:08 PM
nate edited the summary of this revision. (Show Details)Dec 10 2019, 11:20 PM
nate added reviewers: alangenfeld, prha, max, schrockn.
max requested changes to this revision.Dec 11 2019, 10:20 PM

requesting changes because i think we should drop the diagram

docs/sections/deploying/aws.rst
24

what if $DAGSTER_HOME isn't set

39

, launch Dagit as a ...

46

can we provide an example

54

Dagit

55

we should emphasize that this is not in the instance yaml, and maybe add a phrase explaining that/why execution is configurable on a per-pipeline basis

68

you can also do this off aws

90

you'll probably also want to -- b/c everything should work fine with EBS as well

docs/sections/deploying/index.rst
13

we went back and forth quite a bit on diagrams for this and while we don't have a final, accepted version, i think consensus was that some of the semantic slippages that are preserved in this version are misleading -- can we drop this?

17

let's explain why

35

i think this is a little inexact -- pipelines don't include config, etc.

70

this is confusing, we should use the words system storage

docs/sections/deploying/reference.rst
123–149

can we maintain the previous info as well

144

this is confusing to surface in a document at this level

This revision now requires changes to proceed.Dec 11 2019, 10:20 PM
nate marked 13 inline comments as done.Dec 19 2019, 1:22 AM
nate added inline comments.
docs/sections/deploying/aws.rst
24

added explicit setting of $DAGSTER_HOME above

46

hmm. I wasn't even going to mention this file here, since I think it's kind of a distraction, and I don't want the reader to rathole on a file that they probably won't ever have to touch (in 99% of regular use, I don't bother looking at it) - I could just remove this mention? Thoughts?

55

yeah, I agree. I still think this is super confusing (see https://github.com/dagster-io/dagster/issues/1973) - but at least for now I agree that we should clarify the distinction

68

yeah trying to focus on the straight-line path to getting things running vs. presenting diversity of options here, I think we can provide some good discussion of less common patterns in the reference section later

90

cool, done

docs/sections/deploying/index.rst
13

cool, removed

17

I'm going to just remove this sentence along with removing the diagram

35

good call - changed wording to "Any pipelines that are configured with this YAML"

docs/sections/deploying/reference.rst
144

yeah agree, I'm going to remove 3

nate updated this revision to Diff 7965.Dec 19 2019, 1:22 AM
nate marked 9 inline comments as done.

Update based on comments

nate updated this revision to Diff 7978.Dec 19 2019, 2:18 AM

rebase

max accepted this revision.Dec 19 2019, 2:58 AM

do we want to check in the diagram?

docs/sections/deploying/aws.rst
46

maybe just don't mention it then

This revision is now accepted and ready to land.Dec 19 2019, 2:58 AM
nate added a comment.Dec 19 2019, 4:30 AM

do we want to check in the diagram?

nah, let's do some more round with the diagrams you made previously, I think we can continue iterating to get to something that feels good here :)