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

Adding feature stability table to docs and including one example for encryption support in cilium #9555

Merged
merged 8 commits into from
Jul 16, 2020

Conversation

moshevayner
Copy link
Member

Fixes #9151
To get started, I also added this table to the cilium docs for a feature I previously added. This can provide an example for additional features to which we would like to add this table.
Example output of the table when rendered:

image

/cc @rifelpet @olemarkus

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/documentation size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2020
@moshevayner
Copy link
Member Author

I might be missing something, but following the failure output of hack/verify-boilerplate.sh I ran hack/update-header.sh but nothing happens, it doesn't seem to change anything in mkdocs_macro.py.
If I read that script correctly, I think it looks for the copyright block, so I tried adding that manually and it still didn't work.

What am I missing?

mosheshitrit:kops mosheshitrit$ hack/verify-boilerplate.sh
FAIL: Boilerplate header is wrong for: /Users/mosheshitrit/src/kubernetes/kops/src/k8s.io/kops/mkdocs_macros.py
FAIL: Please execute ./hack/update-header.sh
mosheshitrit:kops mosheshitrit$ hack/update-header.sh
mosheshitrit:kops mosheshitrit$ hack/verify-boilerplate.sh
FAIL: Boilerplate header is wrong for: /Users/mosheshitrit/src/kubernetes/kops/src/k8s.io/kops/mkdocs_macros.py
FAIL: Please execute ./hack/update-header.sh

@moshevayner
Copy link
Member Author

OK found it.
Apparently hack/update_header.sh doesn't have a config for .py files even though it does exist under hack/boilerplate folder. Adding a commit to address that.

@olemarkus
Copy link
Member

/lgtm

Why did the netlify deploy cancel though?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2020
@hakman
Copy link
Member

hakman commented Jul 13, 2020

Why did the netlify deploy cancel though?

@olemarkus Most likely because last commit was on a something not tracked by config file:

ignore = "git diff --quiet HEAD^ HEAD netlify.toml Makefile mkdocs.yml docs/ images/"

@MoShitrit any chance mkdocs_macros.py can be moved to the scripts dir?

@moshevayner
Copy link
Member Author

Why did the netlify deploy cancel though?

@olemarkus Most likely because last commit was on a something not tracked by config file:

ignore = "git diff --quiet HEAD^ HEAD netlify.toml Makefile mkdocs.yml docs/ images/"

@MoShitrit any chance mkdocs_macros.py can be moved to the scripts dir?

@hakman Yeah, sure. I think it'll involve some additional config to the plugin but I can do that.
By the scripts dir- do you mean I should create one and put it there? Or is it the hack dir?
I tried to find a dir named scripts but couldn't find any.

@hakman
Copy link
Member

hakman commented Jul 13, 2020

@MoShitrit you are right, meant hack. Thanks!

@moshevayner
Copy link
Member Author

@hakman
Okay, so after re-reading their docs about the location of the module, I think it might work out better to have a dedicated python package under the root directory, if we don't want to have the file located in the same path as mkdocs.yml. So I'm thinking probably creating a python package called mkdocs_macros/ and moving the .py file to this folder. Then we'll have to change the plugin config to point to that directory and that should do it.
WDYT? Is it worth the effort? Or should we just keep it as it is now?
If we do want to use the hack dir, we'll have to convert it to a python package (by adding a __init__.py file as well there. I'm okay with taking that approach as well, but I'm not sure if we should do it, since most of the files there aren't python and aren't really used by mkdocs at all.

@hakman
Copy link
Member

hakman commented Jul 13, 2020

Thanks for checking @MoShitrit . I don't think it's worth the extra work, but let's see if @rifelpet and @mikesplain have some feedback also.

@rifelpet
Copy link
Member

I think I'd lean towards moving mkdocs_macros.py into the hack directory and add a hack/__init__.py that way we avoid yet another file in the repo root. I think if you squash your commits, the netlify job will not be cancelled and we'll be able to check it out in the browser.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 14, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 14, 2020
@moshevayner
Copy link
Member Author

Created a new package under hack called mkdocs_macros and updated mkdocs.yml to point to the new package as the module for Mkdocs-macros plugin.
I see that the netlify deploy preview worked this time, so we can see an example of the table here

@mikesplain
Copy link
Contributor

This looks great, thanks so much for your work on this @MoShitrit!

My only comment is more of a nit, looks like the table goes full with rather than ending at the second column. Any way we can clean this up?

Screen Shot 2020-07-14 at 9 20 46 AM

Other than that, I think this is a great start!

@moshevayner
Copy link
Member Author

Thanks, @mikesplain !
So I tried to look into that table alignment/padding thing, but I think it's more of how Mkdocs renders every custom block (tables, code etc.). I have a strong feeling this comes from the Mkdocs-material theme we're using.
I confirmed that by finding another table in our docs which doesn't use the macros plugin at all, and it looks exactly the same - https://kops.sigs.k8s.io/welcome/releases/#compatibility-matrix
I tried to do some html styling to change that, but it kinda broke it. 😖
I tried to look a little bit a the material theme docs but couldn't find a straightforward answer on how to change that (we can maybe apply some external css, but I feel like it could be a little bit out of scope for this PR and it could take me good bit of time to figure this out, lol)
WDYT?

@mikesplain
Copy link
Contributor

mikesplain commented Jul 14, 2020

Hmm gotcha, you should be able to change max-width: 100%; to max-width: fit-content; on the table object, which worked for me at least. You would likely need to wrap the table in a div with a new class like class=kops_feature_table, then add an override css to extra.css like

.kops_feature_table table {
    max-width: fit-content;
}

But to your point, this is easily fixed in a separate PR if someone spends the time on it. I'm fine without it for now. Thanks for the hard work on this, it's great!

@moshevayner
Copy link
Member Author

Hmm gotcha, you should be able to change max-width: 100%; to max-width: fit-content; on the table object, which worked for me at least. You would likely need to wrap the table in a div with a new class like class=kops_feature_table, then add an override css to extra.css like

.kops_feature_table table {
    max-width: fit-content;
}

But to your point, this is easily fixed in a separate PR if someone spends the time on it. I'm fine without it for now. Thanks for the hard work on this, it's great!

Yas!!! Got it now. I had to do it with a subclass, but your pointer took me to where I needed 😄

image

Pushing up the commit now so you can see the code change.

Thanks for that, @mikesplain ! 🙇‍♂️

@moshevayner
Copy link
Member Author

@moshevayner
Copy link
Member Author

Looking to get a 👍 whenever anyone has a free cycle
@mikesplain @rifelpet

@hakman
Copy link
Member

hakman commented Jul 16, 2020

@MoShitrit could you also add the hack/ dir to this list to trigger docs updates on modules changes?

ignore = "git diff --quiet HEAD^ HEAD netlify.toml Makefile mkdocs.yml docs/ images/"

@moshevayner
Copy link
Member Author

@hakman done! 🚀 😄 d43c9b5

@hakman
Copy link
Member

hakman commented Jul 16, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2020
@moshevayner
Copy link
Member Author

/retest

@rifelpet
Copy link
Member

looks great, thanks again! hopefully we can get this setup with all the other feature docs too.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MoShitrit, rifelpet

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9d675e3 into kubernetes:master Jul 16, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 16, 2020
@moshevayner moshevayner deleted the issue-9151 branch July 16, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Documentation Clarity] Please clarify when specific fields are added to the configuration schemas
6 participants