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
23

what if $DAGSTER_HOME isn't set

38

, launch Dagit as a ...

45

can we provide an example

53

Dagit

54

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

67

you can also do this off aws

89

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

docs/sections/deploying/index.rst
12

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?

16

let's explain why

34

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

69

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

143

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
23

added explicit setting of $DAGSTER_HOME above

45

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?

54

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

67

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

89

cool, done

docs/sections/deploying/index.rst
12

cool, removed

16

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

34

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

docs/sections/deploying/reference.rst
143

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
45

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 :)

This revision was automatically updated to reflect the committed changes.