-
-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Add ProductImage::product_version function #817
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality LGTM, could use a more descriptive PR title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I would like to object this change. Yes, currently we can save ~ 10 CPU cycles :D, but in the future we will have
not only automatic stackable version detection, but the operator might also automatically pick the product version for you (probably from the LTS line).
This might involve network calls or some constants compiled in the code or something completely else.
I would prefer to not build on the fact that the product version determination is currently cheap (n network call) and simple.
For now I would prefer sticking to product_image.resolve().product_version
.
I can revert if this is a veto. No problem. Though I agree that the code comment insinuates this is about saving CPU cycles, it was more about failing fast and having concise code. |
I guess you are right, I forgot about the need of passing I created a PR for a small refactoring, would be awesome if you could have a look! #818 |
Description
Needed for stackabletech/hbase-operator#506
Useful for operators that need to support incompatible product versions.
Definition of Done Checklist