Add documentation to the controller code and main.go #67
Conversation
Adding documentation to the code in 'main.go' so that contributors and developers can understand the purpose of each function, variable, etc..
…ller.go Adding documentation to various functions and variables within the nodefeaturediscovery_controller.go file so that users and contributors can have a deeper understanding of how the reconciliation process works with the NFD Operator.
Adding documentation to describe what each of the vars and funcs in the NFD controls file does, when to use them, and why they're important.
Adding documentation describing the funcs, vars, etc. in the NFD controller resources file so that users and contributors can understand how they all work.
…erator) Adding documentation to the NFD state functions related to NFD itself so that users and contributors can understand how NFD works with the NFD operator, especially if they are looking at another file that references these functions and the NFD struct.
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
|
Hi @courtneypacheco. 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 Once the patch is verified, the new status will be reflected by the 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. |
|
Welcome @courtneypacheco! |
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: courtneypacheco 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 |
Signed-off-by: Courtney Pacheco <cpacheco@redhat.com>
|
CLA has been signed, it should be updated soon |
|
hey @mrbobbytables !!! @courtneypacheco has signed the CLA, could you help us with the flag :) thanks, you are awesome Bob!!! |
|
/check-cla |
4 similar comments
|
/check-cla |
|
/check-cla |
|
/check-cla |
|
/check-cla |
There was a problem hiding this comment.
Welcome @courtneypacheco and thanks for the patchset! Improving the readability and documentation of the code through code comments is great. It's good to have the code thoroughly commented. However, I feel that we might slip to a bit excessive side here and might want to trim the comments a bit 😅 I think there's a fine line (and personal preferences) when there are too much code comments as they need to be maintained, too. They can become a sort of a maintenance burden, and, get out-of-sync with the actual source code. Of course, the ideal would be that all code is so well written that it documents itself and is easy to read without any comments (except for API documentation as "docstrings" before exported symbols). Of course, that's not the case in real world and well-placed, concise comments aid in the readability and document non-obvious/atypical characteristics.
I didn't have the time to go thoroughly through the whole PR, yet. Thus, I just left a few random-ish review comments.
Also, I took a quick look at the commit messages. Looks good, overall 👍 Just few small nits (I know I should be more consistent with commit messages of all PRs but the Github interface does not support that much) 😎 The K8s commit message guidelines is a good read. From there:
- Try to keep the subject line under 72 chars. I know this is often hard but could drop the part in parentheses from the second-last commit, for example
- Use imperative mood: Add documentation instead of Adding documentation
- The first word should be capitalized: Fix gofmt formatting instead of fix gofmt formatting
I'll take a deeper look later 😸
The documentation in the controller and main.go files was unnecessarily long and could pose problems in the future if a maintainer or contributor wants to modify a feature that has a lot of existing documentation. Ideally, documentation should be concise. Signed-off-by: Courtney Pacheco <cpacheco@redhat.com>
|
I signed it |
|
/check-cla |
|
CLA is check now, we are ready for another round @marquiz |
|
/ok-to-test |
👍 Apologies for being inactive on this one. I'll continue the review shortly, probably in smaller chunks, whenever I find adequate time. |
There was a problem hiding this comment.
Another batch of comments. A lot of detailed nitting 🙄 Don't take it too hard @courtneypacheco. You're doing a good job here 😄 I just act as an editor, trying to streamline the comments a bit
|
Release v0.9.0 on NFD is being cooked, so we need to start preparing for Operator V0.3.0, and I want this PR in, how are we here? |
@courtneypacheco how is it? I still need to review the two remaining files, though. Try to do it asap |
Update the NFD documentation based on feedback in the PR. Will squash commits before merging.
|
@marquiz Sorry for the delay. I updated the documentation. I will squash the commits once everything looks good. WDYT? Btw, thank you very much for the feedback! It was really helpful! |
No problem, thanks for the update. I'll check it shortly
Yes, that would be good
Good to hear, glad to help. And, as I said, I hope you don't take it too hard despite all the nitting 😆 |
|
@courtneypacheco: 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. |
There was a problem hiding this comment.
Thanks @courtneypacheco for your efforts 😎 Looks good to me now 👍
Please rebase and squash and then we can merge this (unless @ArangoGutierrez has some feedback)
@courtneypacheco we are good to go, let's squash -> Rebase and we will be good to merge |
|
ping @courtneypacheco |
|
/close |
|
@ArangoGutierrez: Closed this PR. In response to this:
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. |
Adding documentation to the controller code and main.go so that contributors and users have an easy time understanding the purpose of the functions, variables, structs, etc. in the codebase.