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

Remove pkg/ready #3476

Merged
merged 2 commits into from
Jul 21, 2020
Merged

Remove pkg/ready #3476

merged 2 commits into from
Jul 21, 2020

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Jul 20, 2020

Description of the change:
Remove the readiness probe helpers in pkg/ready.

Motivation for the change:
With the new scaffolds the readiness probe helpers in pkg/ready are no longer used by default, and need to be removed from the main CLI repo for the v1.0.0 release.
Related to #3327

TODO

  • Add a new changelog fragment

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.

They are no longer used by default. However, are still they valid/useful with the new layout? Would we really like to no longer support it? If yes, we need to add a fragment with the info for the migration guide since it is a significant breaking change. See that it has been used: https://github.com/search?q=operator-sdk%2Fpkg%2Fready&type=Code

@hasbro17
Copy link
Contributor Author

@camilamacedo86 Per our pervious discussions we decided the readiness probe helpers would be removed in v1.0.0.
It made sense when they were part of our scaffolds but the new plugin scaffolds don't stipulate using readiness probes by default.

And for users who still want it, the helpers are only a few lines of writing/reading a file on disk. Should be easy enough to replicate.

And yes I have a TODO for the fragment which I'll add shortly.

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.

Just missing fragment. Otherwise, shows ok.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
Copy link
Member

@jmrodri jmrodri 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 added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@joelanford
Copy link
Member

Just for reference, we removed usage of the ready package from the Go scaffold all the way back in v0.4.0 because the way we implemented it caused deadlock problems. See #932

Also controller-runtime supports running a readyz server with custom http handlers, which is what users should use going forward. Simply add a healthz.Checker (e.g. healthz.Ping) using manager.AddReadyzCheck

@hasbro17 hasbro17 closed this Jul 21, 2020
@hasbro17 hasbro17 reopened this Jul 21, 2020
@hasbro17 hasbro17 merged commit d4c3303 into operator-framework:master Jul 21, 2020
@hasbro17 hasbro17 deleted the rm-pkg-ready branch July 21, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants