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 Good Practices documentation #3267

Closed

Conversation

Sajiyah-Salat
Copy link
Contributor

@Sajiyah-Salat Sajiyah-Salat commented Mar 7, 2023

Description

Adds new section within Good Practices information

Partial Fixes for #3203

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 7, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @Sajiyah-Salat. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 7, 2023
@@ -7,6 +7,29 @@ This Quick Start guide will cover:
- [Running locally](#test-it-out)
- [Running in-cluster](#run-it-on-the-cluster)

# Overview
Copy link
Member

@camilamacedo86 camilamacedo86 Mar 7, 2023

Choose a reason for hiding this comment

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

It is not a content for the quick start.
The goal of the quick start is only provide the instructions for a quick-start.
We should not have very verbose instructions.

See the comment: #3196 (comment)

The idea is to create a new doc section called overview which would be after the quick-start section ( look at the left side of the menu )

Then, in this section we can add the content.
Also, we should NOT link https://dev4devs.com/2020/08/16/how-to-getting-started-develop-go-operators-from-scratch-with-sdk-1-0/. The idea is to add to this new section a similar content. See that the content/syntax used in the blog is outdated. You can copy and paste and the update / improve it.

Also, in this section we should add the other points add to the comment #3196 (comment). We can also create more one new section called "Common Suggestions" or "Good Practices" and add those items there. Afterwords just link and suggest users check it out after read the overview.

Also, please check the CONTRIBUTING.md guide. All PR's must to have the right emojis we use them to generate the release notes.

I hope that can help out.

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying. I will surely work on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the dev4blog I think there is much info to process in this blog. There are steps tutorial so I dont think I cantake things out of it and add it on blog. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the dev4 blog. The blog is quite of old. it is a tutorial for making a operator sdk and we already have crojob tutorial docs for that. If you want to give blog reference in docs. I have found one https://yash-kukreja-98.medium.com/develop-on-kubernetes-series-operator-dev-the-introduction-1dcbaadc9fa7 WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please clarify like what thing you would like to see in "Common Suggestions" or "Good Practices"

Copy link
Member

@camilamacedo86 camilamacedo86 Mar 14, 2023

Choose a reason for hiding this comment

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

Hi @Sajiyah-Salat,

That still not exactly what s suggested in the comment #3196 (comment). The idea is mainly

  • New doc with the name "Good Practices" and add what you added here.
  • Then, create a new doc, "Overview", which would have similar content https://dev4devs.com/2020/08/16/how-to-getting-started-develop-go-operators-from-scratch-with-sdk-1-0/. You'll need to read, follow the steps and update it according to kubebuilder latest scaffold. Also, it will be required some tweaks and the other links as a reference as well. The goal is NOT to add the link or blog references. The idea is to provide a minimal and fast overview to make the info more accessible to new users and skills up faster in the basic concepts/ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dev4 blog teach us to make a operator. However we already have docs about that. what do you want me to extract from the blog then.

Copy link
Member

Choose a reason for hiding this comment

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

Our tutorials are quite extensive. The aim was to develop a concise overview of the blog, allowing users to grasp the general workings without having to delve deeply into the entire tutorial series. Currently, we lack this condensed document, and as a result, when assisting community members, I typically share the blog for their reference.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 7, 2023
@Sajiyah-Salat
Copy link
Contributor Author

i will update this file as suggested soon.

@Sajiyah-Salat
Copy link
Contributor Author

I will let you know if needed. but here I am waiting for @camilamacedo86 but she seems to be busy.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

@Sajiyah-Salat Please move the current section to a separate page after tutorial and reference the same in the Quickstart section.

Also, I've made changes to the content which includes corrections, some explanations and references to other available documentations.

@@ -0,0 +1,20 @@
# Overview
Copy link
Member

Choose a reason for hiding this comment

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

@Sajiyah-Salat This does not seem to be the content that should immediately appear in the overview section of the KB book. The right way as @camilamacedo86 suggested is to create a separate page, which instead of calling it as "Overview" should be "References" (for example, in SDK documentation we have this that covers the questions/doubts that users may have. That page can then be referenced in the Quickstart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. I will make changes as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

camilla suggested me to give it a name "good practices" so I updated a name. Also in overview section camilla like to see the content of dev4blog which teach us to make a operator. but as long as I know we have good tutorial for that. Help me understand things.

Copy link
Member

Choose a reason for hiding this comment

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

@Sajiyah-Salat,

I suggested:

  • a) We have a new section like "Good Practices" OR "Common Suggestions" under references which would be like what was done here and exist in the SDK docs
  • b) We have another new section like "Overview" (which would have content that is NOT added on this PR) and would be similar to the dev4devs blog provided as an example (Not equal it should NOT be a copy and paste). This doc would have the purpose of doing an overview low verbose before the tutorials to describe how things work to make the user's macro understatement easier and faster.

More info: #3196 (comment)

docs/book/src/Overview Outdated Show resolved Hide resolved
@Sajiyah-Salat Sajiyah-Salat changed the title Update quick-start.md 📖 Update quick-start.md Apr 28, 2023
@Sajiyah-Salat
Copy link
Contributor Author

cc; @camilamacedo86
@varshaprasad96

@@ -7,7 +7,7 @@ This Quick Start guide will cover:
- [Running locally](#test-it-out)
- [Running in-cluster](#run-it-on-the-cluster)

## Prerequisites
## Prerequisites
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this one?

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.

@Sajiyah-Salat,

Following some comments:

  • a) Can you add the section Good Practices under the reference in the left menu and link this one?
  • b) Revert the change in the quick start
  • c) Ensure that the title matches with what was done here. ( I am updating this one for you can check how it works)
  • d) The Description of the PR should describe what was done. It should not have questions or concerns. those can be added via comments in the PR or the issue. (I update this one for you check how it works)
  • e) Ensure that all docs follow the same layout and standards, and see that you have questions within a space between the answers and others not. So, please make sure that everyone has the space.
  • f) It does not close [Doc] - Create doc with common suggestions and overview #3203. It is only a partial delivery of that. We are still missing the OVERVIEW. That would be a section after the Quick Start to give an overview of how things work for end-users
  • g) Do not add the links directly. Links should be added via alias (see the other docs) and then then, the alias should be defined at the bottom of documentation
  • h) We should not use the URL to define links for kubebuilder docs. Instead of that, we should use relative paths (/docs/... /mydoc.md)

Also, I added some PR comments.

@camilamacedo86 camilamacedo86 changed the title 📖 Update quick-start.md 📖 Add Good Practices documentation May 4, 2023

The reconciliation is a continuos loop that performs necessary actions on the current state to ensure that the Custom Resource reaches the desired state as specified by the user.

## Why should reconciliations be idempotent?
Copy link
Member

Choose a reason for hiding this comment

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

IMO we need to define here solutions and not reconciliations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, what solutions you would like to see. will you gide me to reference docs? or elaborate


Writing reconciliation logic according to specific events, breaks the recommendation of operator pattern and goes against the design principles of [controller-runtime](https://github.com/kubernetes-sigs/controller-runtime). This may lead to unforeseen consequences, such as resources becoming stuck and requiring manual intervention.

## Understanding Kubernetes APIs and following API conventions
Copy link
Member

Choose a reason for hiding this comment

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

We are adding two subejcts in the same topic.
IMO we should have one topic per subejct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, this is a good practices section where student get answers to their common question. so adding different questions which contain different topics is completely ok. Also if we add different topics in different page and just reference it in good practices, this will also create confusion for beginners. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

The goal here is not to help students but users.
Also, add more than one subject under the same topic does not seems helpful to provide a better understand and that is my point.


By developing Operator pattern solutions, we extend the Kubernetes api. Understanding the [best practices](https://sdk.operatorframework.io/docs/best-practices/) and following the design patterns is important in the development process.

## Can my controller reconcile more than one GVK?
Copy link
Member

Choose a reason for hiding this comment

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

Can you please look at the SDK doc. https://sdk.operatorframework.io/docs/best-practices/common-recommendation/

The reconciliation can reconcile more the one GKV.
We should avoid a design where the controller is responsible for reconciling more than one primarily KIND

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. The docs have already answer it well. if you want I can add this link so reader can get more idea about it.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 11, 2023
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2023
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2023
@k8s-ci-robot
Copy link
Contributor

@Sajiyah-Salat: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubebuilder-e2e-k8s-1-26-0 1762c92 link true /test pull-kubebuilder-e2e-k8s-1-26-0
pull-kubebuilder-e2e-k8s-1-25-3 1762c92 link true /test pull-kubebuilder-e2e-k8s-1-25-3
pull-kubebuilder-test 1762c92 link true /test pull-kubebuilder-test
pull-kubebuilder-e2e-k8s-1-27-1 1762c92 link true /test pull-kubebuilder-e2e-k8s-1-27-1

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Sajiyah-Salat
Copy link
Contributor Author

Hello @camilamacedo86 can you please help me remove this extra commits.

@Sajiyah-Salat
Copy link
Contributor Author

As seen, I have messed this pr, and I am really sorry for that. started a new one. I will make sure to learn git rebase and squash commands as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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