Skip to content
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

Add documentation for Kubernetes credential management #213

Merged
merged 5 commits into from
Jun 26, 2019
Merged

Conversation

cirocosta
Copy link
Member

fixes #96

Ciro S. Costa and others added 3 commits May 29, 2019 14:45
See #96

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Co-authored-by: Mark Huang <mhuang@pivotal.io>
Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
---
# Identifies the `web` service as an actor.
#
# ref: https://kubernetes.io/docs/reference/access-authn-authz/service-accounts-admin/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vito do you think this kind of comment is too much? Providing references to the document from the official Kubernetes website?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah that's super useful. 👍

- name: CONCOURSE_KUBERNETES_NAMESPACE_PREFIX
value: "myprefix-"
# ...
}}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this YAML file is quite big 😬 I'm not sure if this would be the best way of presenting it - I tried separating them into separate code blocks, but I feel that this way looks a bit better (and it's quite common to see docs like this when it comes to k8s).

wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-05-30 at 15 15 28-fullpage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would an alternative be to split it up and move the comments into the Booklit document? Or is having it all in one YAML document useful for copy-pasting and submitting everything at once?

Copy link
Member Author

@cirocosta cirocosta Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooooh good point, those side comments would be nice. I'll give them a try here 👍

Or is having it all in one YAML document useful for copy-pasting and submitting everything at once?

hmmm I saw a couple of times people having multiple yaml blocks altogether when it comes to k8s, but I'd personally split 😅


(for some reason I thought you mentioned using \aside lol)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after splitting, seems easier to follow; wdyt?

Screen Shot 2019-06-20 at 16 34 44-fullpage


\codeblock{bash}{{{
CONCOURSE_KUBERNETES_NAMESPACE_PREFIX=some-other-prefix-
}}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit divided by letting this configuration piece here - it seemed quite far from the other two configurations at the top of the page 🤔 any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine - it's the same as with the other credential managers. I'm not sure how often folks will set this but since it has a sane default I think it can be "below the fold".

@cirocosta cirocosta marked this pull request as ready for review May 30, 2019 19:17
@cirocosta cirocosta requested a review from vito May 30, 2019 19:17
\title{Configuring Kubernetes RBAC}

As the Web nodes need to retrieve secrets from namespaces that are not its
own, it needs extra permissions to do so.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

web nodes are plural but its and it are singular.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

truuuue, thanks!!

Ciro S. Costa added 2 commits June 20, 2019 16:35
Previously, in the kubernetes RBAC example we had all of the kubernetes
objects needed tied together in a single multi-object yaml block, which
was quite bad to read.

Now, each object has its own descriptions through regular comments,
making it more appealing to read.

Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Signed-off-by: Ciro S. Costa <cscosta@pivotal.io>
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@vito vito merged commit 470941e into master Jun 26, 2019
@vito vito deleted the issue/96 branch June 26, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No documentation for Kubernetes credential manager
3 participants