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

docs: add cilium-operator technical overview documentation #14530

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

fristonio
Copy link
Member

Fixes #13298

@fristonio fristonio added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jan 6, 2021
@fristonio fristonio requested a review from a team as a code owner January 6, 2021 10:15
@fristonio fristonio requested a review from qmonnet January 6, 2021 10:15
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 6, 2021
@fristonio fristonio requested a review from a team as a code owner January 6, 2021 10:27
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Looks great!

Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Copy link
Member

@ungureanuvladvictor ungureanuvladvictor left a comment

Choose a reason for hiding this comment

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

LGTM

Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

This is great! Pointed out mostly minor things for consistency and clarity

Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
@aanm aanm removed their assignment Jan 7, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the doc!
Please find a few nits inline. Additionally,

  • Please make sure to be consistent and use either regular or title case for all titles.
  • Should this document be moved elsewhere than the root of Documentation? Maybe Documentation/concepts/ at least?

Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
Documentation/cilium_operator.rst Outdated Show resolved Hide resolved
@joestringer joestringer moved this from Done to In progress in 1.10.0 Feb 12, 2021
@stale
Copy link

stale bot commented Feb 22, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 22, 2021
@christarazi christarazi added pinned These issues are not marked stale by our issue bot. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Feb 23, 2021
@fristonio fristonio force-pushed the pr/fristonio/operator-docs branch 2 times, most recently from e2c00ee to db6de66 Compare March 1, 2021 17:17
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

It looks like this PR is waiting for my approval to be merged, but I would still appreciate to have

  • Consistent title case on titles (+ consistency on expansion, or not, of “Garbage Collection”),
  • Consistent case on “KVStore”.

Would it make sense to add a directory for documentation oriented towards developers? We probably don't want to move the existing documents bpf, hubble etc.), but that would avoid adding too many new documents to the root of the Documentation/ as this section grows?

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>
@fristonio
Copy link
Member Author

@qmonnet I have updated the docs to have a more consistent format for the specifics you mentioned. Let me know if I missed something.

Would it make sense to add a directory for documentation oriented towards developers?

Yeah, I think it makes sense to do this. I don't have a concrete option for this though, we can possibly have a section inside For Developers called Internals and put hubble and these operator docs under it?

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

we can possibly have a section inside For Developers called Internals and put hubble and these operator docs under it?

Yes, this sounds like a good option to me, let's do this please.

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>
@fristonio
Copy link
Member Author

@qmonnet I have added a section for Cilium components specific internals under For Developers. Let me know if you see a way we can improve it.
image

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, I'm just wondering if we're OK with moving the Hubble doc (I thought you meant reorganising the TOC, I didn't think you would rename the file and change the URL). I think it's fine, because if people want stable links, they should use a version number in the URL, but I'm not entirely sure of our policy on that. I'd appreciate another look from @joestringer, for example.

@joestringer
Copy link
Member

👍 I'm okay with this. We can also set up redirects so that the old link leads to the new location.

@qmonnet
Copy link
Member

qmonnet commented Mar 16, 2021

Ok let's go with this version then 👍
Do you know how we would add a redirect, is it configured in this repo in Documentation/ (I could not find it), or on the hosting platform?

@joestringer
Copy link
Member

It's configured in the hosting platform (https://readthedocs.org/dashboard/cilium/redirects/). I have access, I just need source and destination paths and I can add it. I'm not exactly sure whether it unconditionally redirects or whether it only redirects in the case of direct navigation hitting a 404 so we may need to do a little investigation together there.

@fristonio
Copy link
Member Author

Hey @joestringer
I am happy to help setup the redirect. If setting path based redirect is possible then we need a redirect from /hubble to /internals/hubble. The URL for the two looks will like this for latest version:

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

The Hubble/K8s codeowners are only pulled in because the files are touched, but the validation is more related to docs codeowners. Good to merge.

@joestringer
Copy link
Member

Will just close/reopen to kick GH workflows to double-check that docs tests pass before merge.

@joestringer joestringer reopened this Mar 17, 2021
@joestringer joestringer merged commit fbab3a4 into master Mar 17, 2021
1.10.0 automation moved this from In progress to Done Mar 17, 2021
@joestringer joestringer deleted the pr/fristonio/operator-docs branch March 17, 2021 17:01
@joestringer
Copy link
Member

I set up the redirect, looks like it's working correctly. 👍

@fristonio
Copy link
Member Author

Thanks a lot @joestringer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. pinned These issues are not marked stale by our issue bot. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Improve Cilium operator documentation
7 participants