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

kubebuilder/quickstart: add instruction for enabling status subresource #2897

Merged

Conversation

hasbro17
Copy link
Contributor

Description of the change:
Add docs for enabling status subresource in the kubebuilder quickstart guide.

Motivation for the change:
Resolves the error I saw in #2827 (comment) where the controller failed to update the status subresource with the status writer client.
https://github.com/operator-framework/operator-sdk/blob/master/example/kb-memcached-operator/memcached_controller.go.tmpl#L114

@hasbro17 hasbro17 added the kind/documentation Categorizes issue or PR as related to documentation. label Apr 24, 2020
Copy link
Contributor

@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.

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2020
@hasbro17
Copy link
Contributor Author

@asmacdo Any idea why the netlify checks might have failed?
https://github.com/operator-framework/operator-sdk/pull/2897/checks?check_run_id=613752189
https://app.netlify.com/sites/operator-sdk/deploys/5ea22c115c492f0006ce0ec2

Building sites …
5:01:08 PM: fatal error:
5:01:08 PM: concurrent map read and map write
...

Not sure how to rerun them if this is a flake.

@camilamacedo86
Copy link
Contributor

I saw this error occurs before. No idea too.

@@ -109,6 +109,20 @@ type MemcachedStatus struct {
}
```

Add the `+kubebuilder:subresource:status` [marker comment][status_marker] to enable the [status subresource][status_subresource] for the CRD so that the controller can update the CR status without changing the rest of the CR object:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: wording, comment is implied by marker.

Suggested change
Add the `+kubebuilder:subresource:status` [marker comment][status_marker] to enable the [status subresource][status_subresource] for the CRD so that the controller can update the CR status without changing the rest of the CR object:
Add the `+kubebuilder:subresource:status` [marker][status_marker] to add a [status subresource][status_subresource] to the CRD manifest so that the controller can update CR statuses efficiently:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with the other nits but I'd still want to highlight exactly why we're enabling the status subresource rather than say efficiently. That behavior is not obvious and it's better to be verbose about it.
Part of the reason why I had this error in the first place.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

One nit, otherwise

/lgtm

Copy link
Contributor

@bharathi-tenneti bharathi-tenneti left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@hasbro17 hasbro17 merged commit 0a45049 into operator-framework:master Apr 24, 2020
@hasbro17 hasbro17 deleted the enable-status-subresource branch April 24, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants