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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃摉 Clean up getting started docs #3758

Merged

Conversation

stmcginnis
Copy link
Contributor

While reading through the Getting Started page, I noticed there were several places where things have either changed since it was originally written or incorrect names or references were copy/pasted from other docs. This updates the Getting Started page to accurately reflect what is generated when following the instructions.

There were also several typos and grammatical errors noticed while reading through the doc. Where possible, this was corrected in the Getting Started doc as well as anywhere else in the documentation where this same information was copied.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 29, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 29, 2024

From `testdata/project-v4-with-deploy-image/internal/controller/memcached_controller.go`:
From `internal/controller/memcached_controller.go`:
Copy link
Member

Choose a reason for hiding this comment

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

^ it is the testdata dir of the project where we add the examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as mentioned above, that content is different than these instructions result in. The locally generated content from following the commands above are under the updated path. If we want to refer to the git repo for additional examples I think that could work. But as this part is, it can be confusing referring to a path that is not part of the local content.

Copy link
Member

Choose a reason for hiding this comment

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

I see; we should actually generate the sample under the docs ( with the same instructions ) as we do for the tutorials and just use the code directly from the sample.

So, that when we run make generate we will be able to keep this one update.

See:

Otherwise, it is hard to keep it maintained.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to as @camilamacedo86 said. If we could directly refer the paths in the doc, that would make it easier for us to maintain it. Huge + for the idea though. We would have to probably discuss more on the how the testate would be structured to refer multi-version/multi-group scenarios for the same plugins as we have today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked in to using literatego and making this more integrated, but there were some issues with that approach. The biggest challenge is that pulls in the entire source file, when in most cases here we only want to highlight a small portion of it.

I hope I've reached a happy medium. I do now generate testdata for the getting started docs, and I try to directly pull in relevant content where possible. For the other instances, I just updated the snippet of code the docs were highlighting, but also added a link to the file in the repo so the user has a canonical reference for anything being covered. Then if they do diverge at some point, it's at least possible to follow the link to see the current full example that should match what they get locally by running the commands.

Let me know if there are any better ideas or other ways to improve that.

@@ -33,7 +33,7 @@ kubebuilder init --domain=example.com
Next, we'll create a new API responsible for deploying and managing our Memcached solution. In this instance, we will utilize the [Deploy Image Plugin][deploy-image] to get a comprehensive code implementation for our solution.

```
kubebuilder create api --group example.com --version v1alpha1 --kind Memcached --image=memcached:1.4.36-alpine --image-container-command="memcached,-m=64,-o,modern,-v" --image-container-port="11211" --run-as-user="1001" --plugins="deploy-image/v1-alpha" --make=false
kubebuilder create api --group cache --version v1alpha1 --kind Memcached --image=memcached:1.4.36-alpine --image-container-command="memcached,-m=64,-o,modern,-v" --image-container-port="11211" --run-as-user="1001" --plugins="deploy-image/v1-alpha" --make=false
Copy link
Member

Choose a reason for hiding this comment

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

The example under test data is not built with the group cache. Is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following the instructions here in the getting started guide, this is causing a mismatch between what the resulting local files contain and what the doc later refers to. So yes, this is different than the test data, but the contents of this doc are all different.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I am OK with the change.
Thank you for looking at this one !!!
Your help is very appreciated.

@camilamacedo86
Copy link
Member

Hi @stmcginnis,

Really, thank you for all your collaboration 馃
Just a few nits.

  • In this PR we are addressing more than one thing. For example, the changes in the customize/v2 have no relation with getting started docs. Could you please create a PR for this only?
  • Regarding the required fixes in the Getting Started doc, I think we need to properly sort it out by having the sample under docs so that it will be properly updated. See: 馃摉 Clean up getting started docs聽#3758 (comment) if you be able to do that it would be great

docs/book/src/getting-started.md Outdated Show resolved Hide resolved

From `testdata/project-v4-with-deploy-image/internal/controller/memcached_controller.go`:
From `internal/controller/memcached_controller.go`:
Copy link
Member

Choose a reason for hiding this comment

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

+1 to as @camilamacedo86 said. If we could directly refer the paths in the doc, that would make it easier for us to maintain it. Huge + for the idea though. We would have to probably discuss more on the how the testate would be structured to refer multi-version/multi-group scenarios for the same plugins as we have today.

@stmcginnis
Copy link
Contributor Author

Thanks for the feedback - I will get an updated PR shortly.

One thing about the reference path though. There seems to be a disconnect in what this doc is doing, so I think we need to clarify that. As it is, the getting started guide is a tutorial/guide walking the reader through running commands and working with the generated code based on that. So I think it would be confusing to mix this "real world" walkthrough of generating the code with linking off to a static reference in the repo of a similar-but-not-quite-the-same example of what they have locally.

In other words, I think we need to keep the tutorial separate from and test data reference we could point them at.

We may want to think about changing what this guide is if we just want to refer to examples in the repo. As it is now, there are other issues with the getting started doc. The main one being that the beginning of the doc states what the user will go through and end up with at the end, but then the content of the doc only goes a short way towards those stated goals and stops. So there are definitely bigger issues that need to be decided here.

I see two main choices at this point:

  1. Finish implementing the getting started guide to actually walk the user through all the declared goals, keeping with the locally generated content that the instructions generates for them; or
  2. Change the getting started guide to be a description of what they can find as static content already in the repo, only explaining what local commands they could have run to get similar results on their own.

As it is now, it's a bit incomplete and disjointed.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Jan 30, 2024

Hi @stmcginnis,

There seems to be a disconnect in what this doc is doing, so I think we need to clarify that

That is all point. The tutorial describes how to create the project using the Deploy Image plugin
The examples should be exactly the code that is generated in the project.
Because of that IHMO, the right way to sort it out is by doing as we do in all other places: #3758 (comment)

All that we need to explain is scaffold when we create the API using the plugin.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2024
@stmcginnis
Copy link
Contributor Author

Not sure about the verification test failure. Passing locally for me, so I'm not sure what is different in my environment. Will try to investigate further.

@camilamacedo86
Copy link
Member

Hi @stmcginnis,

GREAT WORK !!!

Not sure about the verification test failure. Passing locally for me, so I'm not sure what is different in my environment. Will try to investigate further.

It is because it is not rebased with the master branch
Just rebase your branch with master, re-run, make generate and then squash the commits, for we have just 1, and it is great to fly (we can get it merged. It will pass in all tests) !!!!

Add generation of getting started project code

This adds a step to generate the code used in the Getting Started docs
page. This allows us to reference the pre-generated code relative to the
docs page so we can keep things in sync between the generated code and
the examples given in the docs.

Fix various typos and grammar in v1alpha1 template

Addresses various spelling and grammar mistakes found in the
template used to generate the v1alpha1 sample projects.

Addresses various typos, grammatical errors, and other refinement
in the Getting Started docs page.

Where possible, source files from the generated testdata project are
imported directly in the docs so they stay up to date. In other areas
where it wasn't practical to pull in the entire source file, this
updates the code snippet shown to match the generated source and
provides a link to the GitHub repo to use as a reference.

Signed-off-by: Sean McGinnis <sean.mcginnis@gmail.com>
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

/approved

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, stmcginnis

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 Jan 31, 2024
@camilamacedo86
Copy link
Member

Hi @stmcginnis

Thank you a lot for your contribution 馃
It is a very nice one.

@k8s-ci-robot k8s-ci-robot merged commit a5b95e3 into kubernetes-sigs:master Jan 31, 2024
19 checks passed
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. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants