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

Describe the status contract #198

Merged
merged 1 commit into from Mar 18, 2022

Conversation

mamachanko
Copy link
Collaborator

@mamachanko mamachanko commented Mar 18, 2022

Since it's not immediately clear how reconciler-runtime expects a resource's .status to look like, I thought I'd propose this for the README.md and api.Status's doc comment.

(reopened after going through the CLA process)

@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #198 (6e6e349) into main (4b49fe7) will not change coverage.
The diff coverage is n/a.

❗ Current head 6e6e349 differs from pull request most recent head 12382e5. Consider uploading reports for the commit 12382e5 to get more accurate results

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   64.71%   64.71%           
=======================================
  Files           8        8           
  Lines         768      768           
=======================================
  Hits          497      497           
  Misses        256      256           
  Partials       15       15           
Impacted Files Coverage Δ
apis/status_types.go 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b49fe7...12382e5. Read the comment docs.

Copy link
Contributor

@scothis scothis 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 awesome, thanks. I left a few suggestions to fix quoting in the json tags.

In practice, I expect resources will have other fields on their status and the direct use will not be common. I'd lean towards removing that example in favor of a custom type the inlines our fields. I don't object to the direct usage example if you think that's common enough.

apis/status_types.go Outdated Show resolved Hide resolved
apis/status_types.go Outdated Show resolved Hide resolved
apis/status_types.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mamachanko
Copy link
Collaborator Author

I left a few suggestions to fix quoting in the json tags.

Thank you! I really botched them all. 🤣

In practice, I expect resources will have other fields on their status and the direct use will not be common. I'd lean towards removing that example in favor of a custom type the inlines our fields. I don't object to the direct usage example if you think that's common enough.

That sounds good to me. I changed it accordingly.

README.md Outdated Show resolved Hide resolved
@mamachanko
Copy link
Collaborator Author

Rebased and fixed up suggestion

@scothis scothis merged commit b435360 into vmware-labs:main Mar 18, 2022
@mamachanko mamachanko deleted the better-status-docs branch March 19, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants