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

📖 Revamp "Watching Resources" documentation for accuracy and clarifty #4170

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 15, 2024

Closes: #4109

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 15, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 15, 2024
@camilamacedo86
Copy link
Member Author

Hi @mogsie

Thank you a lot for raise your concern.
It is all fixed now, feel free to give a look and your help on review will be well appreciated.

docs/book/src/reference/watching-resources.md Outdated Show resolved Hide resolved
return []reconcile.Request{
{
NamespacedName: types.NamespacedName{
Name: "backupbusybox", // Reconcile the associated BackupBusybox resource

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the default scaffold now we add the name. Why?
If we have 2 versions of the same CR such as we have in the tutorial and a controller for each one of them then it will not work out. Also, if we are using the multigroup layout and then, we have the same Kind name and its controllers but for diff groups the same issue will occurs.

See: #4162

@camilamacedo86 camilamacedo86 force-pushed the predicates-doc branch 4 times, most recently from f19cdd9 to ed5a026 Compare September 17, 2024 11:43
@camilamacedo86
Copy link
Member Author

Hi @mogsie

thank you a lot for your time.
I did many changes based on your feedback review. so because of that I close the comments
Please, feel free to check it out
Your amazing help is very well appreciate !!!

Copy link
Contributor

@mogsie mogsie 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! Very clear explanation of the different terms and it's all much more consistent. A few minor nits, and one bigger suggestions, but all in all awesome! ✨

docs/book/src/reference/watching-resources.md Outdated Show resolved Hide resolved
docs/book/src/reference/watching-resources.md Outdated Show resolved Hide resolved
docs/book/src/reference/watching-resources.md Show resolved Hide resolved
docs/book/src/SUMMARY.md Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

At the bottom of this file, I would insert a sentence about how it's common to use this to watch referenced objects, and how EnqueueRequestsFromMapFunc works.

e.g. something like:

A common pattern is to use this when a primary resource contains links to secondary resources. for example, a BusyBox might reference a "script" ConfigMap. If the ConfigMap changes, the BusyBox instance should be reconciled.

The EnqueueRequestsFromMapFunc is called whenever a potentially linked Secondary resource (here, the ConfigMap) is changed somehow. The EnqueueRequestsFromMapFunc needs to find all Primary resources (BusyBox) that reference it. The function passed in to EnqueueRequestsFromMapFunc typically works like this:

  • Let's say a Secondary object (e.g. a ConfigMap) has been modified.
  • The function should return all of the Primary resources (BusyBox) that link to that ConfigMap.
  • To do this it lists all Primary (BusyBox) resources in the same namespace (for security purposes, references should not cross namespaces).
  • For each Primary object, it checks in their spec for the reference to this ConfigMap object.
  • If the primary object does link to the given secondary object, the primary object's name is returned.

The result is then that whenever an object links to another object, and the linked object is modified, then the objects that link to it will be reconciled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hun, acctually how it works is

  • If the resource is OWNED / CREATED by the controller then it should be marked with controllerutil.SetControllerReference and we should watch with Owned
  • If the resource is NOT Owned then we need to use EnqueueRequestsFromMapFunc to watch

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole section is about your second bullet point. The ConfigMap in the above example is a referenced (not owned) resource (i.e. it is found somewhere in the primary object's spec). I am just suggesting that this (common?) use case is mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the document "How to watch resources not owned", we're using another CRD as an example. However, we also mention that a Core type can be used instead.

Why use another CRD? This is because one of the most common questions we receive is: "Can I use CRDs from one project in another?". By using a CRD in the example, we address this frequent concern directly. Additionally, the example of watching owned resources already covers Core types, so it makes sense to diversify the examples.

I'll go ahead and merge this. If anyone (or you) has suggestions for improvements, feel free to open a PR with your ideas. It sometimes is easier then try to explain the idea. I hope this approach works better.

thank you so much for your review.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

- Fully revamped the "Watching Resources" section for improved clarity and accuracy.
- Ensured terminology aligns with Kubernetes and controller-runtime standards.
- Provided detailed examples for watching owned resources, non-owned resources, and applying predicates to refine watches.
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit c00db6e into kubernetes-sigs:master Sep 21, 2024
18 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. release-blocker 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.

Review Watching Resources Docs
3 participants