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

Charm pages #296

Merged
merged 39 commits into from
Mar 2, 2020
Merged

Charm pages #296

merged 39 commits into from
Mar 2, 2020

Conversation

evilnick
Copy link
Collaborator

@evilnick evilnick commented Oct 4, 2019

No description provided.

@evilnick
Copy link
Collaborator Author

A variant -

image

@hyperbolic2346
Copy link
Contributor

🍲

@evilnick
Copy link
Collaborator Author

@tvansteenburgh These are the current variants for the semi-auto-generated charm pages:

Option 1 - Write out in full - This is very similar to the current pages on jaas.ai, it just lists all the config options with a header, type, default and description.

Pros - easy to generate reliably, easy to search, somewhat easy to mix content
Cons - it is a big page. For the more complex charms, it will be frighteningly long and will probably need a TOC.

preview: https://5e319f8d06ca5b00085d48a2--cdk-docs-next.netlify.com/kubernetes/docs/charm-kubernetes-worker-a


Option 2 - tables - This is a semi-intelligent table layout where bigger entries are linked out to notes below.
Pros - quite easy to see most things at-a-glance, more compact than option 1
Cons - slightly confusing to go back and forth between the table and the notes

preview: https://5e319f8d06ca5b00085d48a2--cdk-docs-next.netlify.com/kubernetes/docs/charm-kubernetes-worker-b


Option 3 - foldouts - This hides all the details behind an expander.

Pros - very compact, even for larger amounts of stuff. Doesn't need a TOC
Cons - not searchable in page (things can only be found if they are displayed), may introduce some challenges for autogeneration.

preview: https://5e319f8d06ca5b00085d48a2--cdk-docs-next.netlify.com/kubernetes/docs/charm-kubernetes-worker-c

@kwmonroe
Copy link
Member

My $0.02, the cons for options 1 and 3 are annoying enough to rule them out.

For option 2, is it possible to generate anchors for the See notes links and then add a Back to table link beside each note that takes the user back to the respective table anchor?

Also for 2, is it possible to put all the notes under a collapsed Notes foldout? If the See notes link can automatically unfold and take the user to the appropriate note, having all the notes collapsed by default would would help shorten the page.

Even if the above aren't feasible, I like option 2 the best.

@johnsca
Copy link
Contributor

johnsca commented Jan 29, 2020

I don't find the cons from option 3 as big of a deal as @kwmonroe, since in theory the config option name ought to be sufficiently searchable, and I like the compactness of it. But I'm also fine with option 2 + his suggestion on reverse anchors.

@tvansteenburgh
Copy link
Contributor

@evilnick Thanks for the work that's gone into this so far, and for presenting multiple options. I personally prefer option 2, and it's not close - I think it looks the best. I also like @kwmonroe's idea for the reverse anchors to take one back to the table.

Originally I was thinking that we'd need to be able to add content to accompany the charm config descriptions. Like longer explanations, examples, etc. - stuff that would help explain how to use that config option, but that was too lengthy to put in the actual description field in config.yaml.

But now that I see how nicely the descriptions are rendered, I'm thinking we should in fact put everything in the description field in config.yaml. Interested in others' thoughts on this.

@evilnick
Copy link
Collaborator Author

evilnick commented Feb 7, 2020

The core charms have all been updated. It would be helpful if you could report any weirdness in the tables or the notes below as a comment here

I have made a temporary index page so the easiest way to see each page is from here:
https://deploy-preview-296--cdk-docs-next.netlify.com/kubernetes/docs/charm-reference

The rest of the page is populated with the current README files from the charm store, some of them will need a bit of work also but for now I want to get the tables sorted.

@evilnick
Copy link
Collaborator Author

evilnick commented Feb 7, 2020

Oh, N.B., some of them don't have config options (e.g. kata, easyrsa) so there are no tables on those pages

Copy link
Contributor

@tvansteenburgh tvansteenburgh left a comment

Choose a reason for hiding this comment

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

Reviewed charm-aws-iam, charm-easyrsa, charm-keepalived.

pages/k8s/_site/charm-aws-iam.html Outdated Show resolved Hide resolved
pages/k8s/_site/charm-aws-iam.html Outdated Show resolved Hide resolved
pages/k8s/charm-easyrsa.md Outdated Show resolved Hide resolved
pages/k8s/charm-easyrsa.md Outdated Show resolved Hide resolved
pages/k8s/charm-easyrsa.md Outdated Show resolved Hide resolved
pages/k8s/charm-keepalived.md Outdated Show resolved Hide resolved
pages/k8s/charm-keepalived.md Outdated Show resolved Hide resolved
pages/k8s/charm-keepalived.md Outdated Show resolved Hide resolved
pages/k8s/charm-keepalived.md Outdated Show resolved Hide resolved
pages/k8s/charm-keepalived.md Outdated Show resolved Hide resolved
pages/k8s/charm-flannel.md Outdated Show resolved Hide resolved
pages/k8s/charm-flannel.md Outdated Show resolved Hide resolved
pages/k8s/charm-flannel.md Outdated Show resolved Hide resolved
Copy link
Contributor

@johnsca johnsca left a comment

Choose a reason for hiding this comment

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

Reviewed the integrator charms. They all had out-dated references to CDK, but it was easier to just apply the edits directly than comment on each one, so they're all fixed. +1 for those.



<!-- CONFIG STARTS -->
<!--AUTOGENERATED CONFIG TEXT - DO NOT EDIT -->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand why this config section comes before the examples whereas the other charms have the config last, but it was a bit jarring when flipping between them to see the config reference mixed into the middle. I don't know that there's a reasonable way to get that arrangement for this particular charm, though, because of the additional required config.

Copy link
Member

@kwmonroe kwmonroe left a comment

Choose a reason for hiding this comment

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

Not sure if these links were skipped on purpose, but there are a couple that don't point to the respective charm-$foo pages.

pages/k8s/charm-reference.md Outdated Show resolved Hide resolved
pages/k8s/charm-reference.md Outdated Show resolved Hide resolved
Copy link
Member

@kwmonroe kwmonroe left a comment

Choose a reason for hiding this comment

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

We're inconsistent with the # Contact section in a few of our charms. Most don't have it, so I think these should be removed across the board. They either point to these docs or a charmstore readme, which doesn't seem very useful considering the person reading these pages is already here.

pages/k8s/charm-containerd.md Outdated Show resolved Hide resolved
pages/k8s/charm-docker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kata.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
Copy link
Member

@kwmonroe kwmonroe left a comment

Choose a reason for hiding this comment

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

Requesting changes for the k8s-master charm. Mostly outdated links and CDK vs CK issues.

pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-master.md Show resolved Hide resolved
Copy link
Member

@kwmonroe kwmonroe left a comment

Choose a reason for hiding this comment

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

A few tweaks for k8s-worker. Match k8s-master descriptions where relevant and rephrase some confusing bits.

pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
pages/k8s/charm-kubernetes-worker.md Outdated Show resolved Hide resolved
@evilnick evilnick changed the title WIP - Add framework for charm config Charm pages Feb 25, 2020
@Cynerva
Copy link
Contributor

Cynerva commented Feb 25, 2020

I've re-reviewed the Flannel, Calico, Canal and Tigera Secure EE pages. Looks like all my concerns were indeed resolved. Thanks!

@kwmonroe
Copy link
Member

I made a quick update directly to this branch for the k8s-master resources link. All other issues with my charms (charm-docker, charm-docker-registry, charm-kubeapi-load-balancer, charm-kubernetes-[master|worker]) have been resolved. Thanks!

@evilnick evilnick merged commit 5d227db into charmed-kubernetes:master Mar 2, 2020
@evilnick evilnick deleted the charmconfig branch March 2, 2020 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants