-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: Improve docs #8562
docs: Improve docs #8562
Conversation
Signed-off-by: Alex Collins <[email protected]>
@@ -4,14 +4,16 @@ on: | |||
push: | |||
branches: | |||
- master | |||
pull_request: |
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.
run on PRs
@@ -140,12 +140,6 @@ define protoc | |||
|
|||
endef | |||
|
|||
.PHONY: build |
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.
defunct targets that should never be run
@@ -9,7 +9,7 @@ function check-used { | |||
var="${x%\`}"; | |||
var="${var#\`}"; | |||
if ! grep -qR --exclude="*_test.go" "$var" ./cmd/workflow-controller ./workflow ./persist ./util ./server ; then | |||
echo "Documented variable $var in docs/environment-variables.md is not used anywhere"; | |||
echo "❌ Documented variable $var in docs/environment-variables.md is not used anywhere" >&2; |
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.
clearer success/failure warnings
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@@ -454,15 +440,6 @@ dist/argosay: | |||
mkdir -p dist | |||
cp test/e2e/images/argosay/v2/argosay dist/ | |||
|
|||
.PHONY: pull-images |
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.
defunct
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins [email protected]
As a reviewer, I don't expect you to open and examine every single file because there are too many. Instead I only expect provide high-level feedback:
(a) Are you happy with the problems and solution below? What should be done different?
(b) How does the site look to you? What would you change?
Comments section:
Problem
Engineers are submitting PRs that have not been tested, listed, or had code generation run. This wastes both theirs and the reviewers time resolving, and delaying the PR merge.
Solution
Install a Git pre-commithook that complain if
make pre-commit -B
andmake test
have not been run in the last 15m.Problem
Users often submit PRs that break the docs. This is because they are not checked by CI, and not easy to test locally. These breakages escape into
master
. Commonly this results in missing docs, or docs that look wrong. These typically need to be fixed by a maintainer.Solution
Build the docs on CI so (a) the build fails if the docs are broken and the PR cannot be merged (b) the PR reviewer can check them prior to merge.
This includes:
Problem
Users are struggling to get started, and generally dislike the docs.
Solution
ℹ️ This PR is pushed to origin rather than a fork because Github Actions will not run changed workflows on forks.