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.

Sign up for GitHub

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

Jump to bottom

📖 Good practices Doc page created #3478

Closed
wants to merge 0 commits into from
Closed

📖 Good practices Doc page created #3478

wants to merge 0 commits into from

Conversation

Sajiyah-Salat
Copy link
Contributor

Description
Adds new section within Good Practices information

Partial Fixes for #3203
This pr is a duplicate from #3267 pr as I have messed the squashing and rebasing part.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 27, 2023
@Sajiyah-Salat Sajiyah-Salat changed the title book Good practices Doc page created 📖 Good practices Doc page created Jul 1, 2023
camilamacedo86
camilamacedo86 reviewed Jul 7, 2023
View reviewed changes
docs/book/src/reference/good-practices.md Outdated
Comment on lines 3 to 7
## What is "Reconciliation" in Operators?

When you create a project using Kubebuilder, see the scaffolded code generated under `cmd/main.go`. This code initializes a [Manager][controller-runtime-manager], and the project relies on the [controller-runtime][controller-runtime] framework. The Manager manages [Controllers][controllers], which offer a reconcile function that synchronizes resources until the desired state is achieved within the cluster.

Reconciliation is an ongoing loop that executes necessary operations to maintain the desired state, adhering to Kubernetes principles, such as the [control loop][k8s-control-loop]. For further information, check out the [Operator patterns][k8s-operator-pattern] documentation from Kubernetes to better understand those concepts.
Copy link
Member

Choose a reason for hiding this comment

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

The reconciliation is the control loop code implementation that we do in the controllers which leverage in controller- runtime. But the text does not seems explain it properly. Can we remove 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.

I think to replace it with new defination would be the good idea?I didnt find any reconcilation defination in kubebuilder documentation. I know users coming here will have basic idea about it. But adding it would increase the chances of understanding things better. WDYT?

camilamacedo86
camilamacedo86 reviewed Jul 7, 2023
View reviewed changes
docs/book/src/reference/good-practices.md Outdated
</aside>

[docs]: ./cronjob-tutorial/gvks.html

Copy link
Member

Choose a reason for hiding this comment

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

can we remove the spaces?

varshaprasad96
varshaprasad96 approved these changes Jul 13, 2023
View reviewed changes
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.

The changes look good to me apart from a nit.
Will leave it for @camilamacedo86 to lgtm before merging.

docs/book/src/reference/good-practices.md Outdated Show resolved Hide resolved
@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 13, 2023
@camilamacedo86 camilamacedo86 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 14, 2023
camilamacedo86
camilamacedo86 approved these changes Jul 14, 2023
View reviewed changes
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.

we should only get merged PRs which are with the commits squashed to ensure that the history will be properly tracked.

Since I know that you @Sajiyah-Salat are working in this one for a while and you were facing issues to squash I am moving forward using the option to automatically merge with squash which is not ideal and we can tackle the nits in a fallow up.

Thank you for ALL your hard work on this one 🥇

/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 14, 2023
@camilamacedo86
Copy link
Member

/test pull-kubebuilder-e2e-k8s-1-26-0

camilamacedo86
camilamacedo86 reviewed Jul 14, 2023
View reviewed changes
docs/book/src/reference/good-practices.md Outdated

## Why You Should Adopt Status Conditions

We recommend you manage your solutions using Status Conditionals following the [K8s Api conventions][k8s-aoi-convetions] because:
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 missing the link

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 will add it. Ah! the name is wrong as well. Will fix this soon!

@camilamacedo86
Copy link
Member

/test pull-kubebuilder-e2e-k8s-1-26-0

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added 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. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 15, 2023
@Sajiyah-Salat
Copy link
Contributor Author

Sajiyah-Salat commented Jul 15, 2023 •
edited

when I just check out to my branch there were bunch of files deleted which was not done by me. I was trying to squash the changes but there were uncommit files so I just commit them and these resolve conflict occur. What should I do?

@Sajiyah-Salat Sajiyah-Salat force-pushed the good-prac-final branch 2 times, most recently from 504e37d to 68a29e4 Compare July 24, 2023 12:01
@varshaprasad96
Copy link
Member

@Sajiyah-Salat looks like the PR has conflicts. What it means is - you have made changes in some files, and before you merge it to master, someone else has also made changes in the same files. This means, we need to resolve those conflicts manually. Here is some documentation on how to do so: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. Just to highlight, the overall steps would be:

  1. Pulling the latest changes from master/main branch.
  2. Running a rebase with git rebase command.
  3. Rejecting or accepting the changes you need.
  4. Committing the final changes.

Feel free to let us know if you have any problems while solving conflicts.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed 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. labels Jul 25, 2023
@Sajiyah-Salat
Copy link
Contributor Author

Sajiyah-Salat commented Jul 25, 2023 •
edited

I resolved the conflicts files should I wait for the test to pass or should I squash the commits first?

@Sajiyah-Salat
Copy link
Contributor Author 

docs/book/src/cronjob-tutorial/testdata/project/*: Permission denied
warning: unable to access '.git/rebase-merge/rewritten-pending': Permission denied
error: update_ref failed for ref 'refs/heads/good-prac-final': cannot lock ref 'refs/heads/good-prac-final': is at 4d5f3494fd01d789b30109a3028b9685c96ea036 but expected 4f4d8078b1a1217e637d28d3e81b4dc6f19dff11
error: could not update refs/heads/good-prac-final

The star is added by me which says includes all the files of project dir. The log was long enough stating all the files differently. When I resolved the conflicts by undo the deleted files. I got into this trouble.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86, Sajiyah-Salat, varshaprasad96

The full list of commands accepted by this bot can be found 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 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 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 c811b20 link true /test pull-kubebuilder-e2e-k8s-1-26-0
pull-kubebuilder-e2e-k8s-1-27-1 c811b20 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

@everettraven everettraven Awaiting requested review from everettraven

Assignees

@camilamacedo86 camilamacedo86

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Sajiyah-Salat @camilamacedo86 @k8s-ci-robot @varshaprasad96