-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: rework documentation #328
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some minor suggestions inline, but I'm fine with this landing as it stands.
glide install --strip-vendor | ||
glide-vc --use-lock-file --no-tests --only-code | ||
bazel run //:gazelle | ||
./build.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we also pointed folks at pre-built releases, but we can come back and update this once we have some ;).
docs/dev/environment-variables.md
Outdated
@@ -0,0 +1,15 @@ | |||
# Environment Variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are user docs; folks could be interested even if they only run but never compile the installer. Maybe put them in docs/environment-variables.md
or docs/user/environment-variables.md
?
docs/design/assetgeneration.md
Outdated
## Goal | ||
|
||
Define generation of assets of varied types in installer based on a dependency graph. | ||
The installer internally uses a directed acyclic graph to represent all of the assets it creates as well as their dependencies. This process looks very similar to nearly every decent build system (e.g. bazel, make). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "decent" seems a bit fiesty (are other build systems indecent?). Maybe just say "similar to many build systems (Bazel, Make, etc.)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a ton of build systems that I wouldn't call decent. I'll drop that qualifier though ;)
docs/design/assetgeneration.md
Outdated
|
||
![Image depicting the resource dependency graph](./resource_dep.svg) | ||
|
||
This graph is generated by `dot` from the [source](resource_dep.dot). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked having the full command so I could copy/paste when regenerating the file. Maybe we should have a Makefile
just for the SVG ;).
docs/design/assetgeneration.md
Outdated
|
||
The following graph shows the relationship between the various assets that the installer generates: | ||
|
||
![Image depicting the resource dependency graph](./resource_dep.svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency with your [source](resource_dep.dot)
below, I'd rather drop the ./
here.
docs/dev/environment-variables.md
Outdated
| `OPENSHIFT_INSTALL_PASSWORD` | | ||
| `OPENSHIFT_INSTALL_BASE_DOMAIN` | | ||
| `OPENSHIFT_INSTALL_CLUSTER_NAME` | | ||
| `OPENSHIFT_INSTALL_PULL_SECRET` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we alphabetize this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe also break them up into categories: common env vars and then sections of env vars specific to target platforms
This updates the documentation to reflect the new installer.
@wking Updated to address your comments. |
The installer generates assets based on the dependency [graph](./dependency.md). To generate an asset, all its dependencies have to be generated. The installer needs to define how assets declare this dependency and how it resolves the graph when asked to generated specific targets. Targets are chosen assets that need to be generated, the installer generates all assets that the target depends on and writes to disk only the targets, consuming any inputs from disk or state from a previous run. | ||
The installer generates assets based on the [dependency graph](#dependency-graph). Each asset seperately defines how it can be generated as well as its dependencies. Targets represent a set of assets that should be generated and written to disk for the user's consumption. When a user invokes the installer for a particular target, each of the assets in the set is generated as well as any dependencies. This eventually results in the user being prompted for any missing information (e.g. administrator password, target platform). | ||
|
||
The installer is also able to read assets from disk if they have been provided by the user. In the event that an asset exists on disk, the install won't generate the asset, but will instead consume the asset from disk (removing the file). This allows the installer to be run multiple times, using the assets generated by the previous invocation. It also allows a user to make modifications to the generated assets before continuing to the next target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consume? Why? Personally I really like the "git-ops" model where one stores configuration files in git and code reacts to commits. I'd expect people want to store their install configs in git and not have to undo the installer deleting things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consume? Why?...
+1 to this, but the installer doesn't do any disk-reading now, so we can revisit when we add that.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crawford, wking The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This restores the console docs which we'd removed from the README in feb41e9 (docs: rework documentation, 2018-09-24, openshift#328). And it moves the kubeconfig location information over from the libvirt-specific docs. Launching the cluster is nice, but these other operations are important too ;). Putting them in the README makes increases their visibility. It also lets us drom them from the libvirt-specific docs, now that the libvirt docs link to the README quick-start for these generic operations.
This restores the console docs which we'd removed from the README in feb41e9 (docs: rework documentation, 2018-09-24, openshift#328). And it moves the kubeconfig location information over from the libvirt-specific docs. Launching the cluster is nice, but these other operations are important too ;). Putting them in the README makes increases their visibility. It also lets us drom them from the libvirt-specific docs, now that the libvirt docs link to the README quick-start for these generic operations.
This updates the documentation to reflect the new installer.