Skip to content

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

Merged
merged 3 commits into from
Jun 26, 2024
Merged

Conversation

razvan
Copy link
Member

@razvan razvan commented Jun 26, 2024

Description

Needed for stackabletech/hbase-operator#506

Useful for operators that need to support incompatible product versions.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author
- [x] Changes are OpenShift compatible
- [x] CRD changes approved
- [x] Integration tests passed (for non trivial changes)
# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)
# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

@razvan razvan requested a review from a team June 26, 2024 14:42
@razvan razvan enabled auto-merge June 26, 2024 15:03
Copy link
Member

@nightkr nightkr left a 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

@razvan razvan added this pull request to the merge queue Jun 26, 2024
Merged via the queue into main with commit f8353ce Jun 26, 2024
17 checks passed
@razvan razvan deleted the feat/misc-utils branch June 26, 2024 15:24
@sbernauer sbernauer self-requested a review June 27, 2024 06:58
@sbernauer sbernauer changed the title feat/misc utils feat: Add ProductImage::product_version function Jun 27, 2024
Copy link
Member

@sbernauer sbernauer left a 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.

@razvan
Copy link
Member Author

razvan commented Jun 27, 2024

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.

@sbernauer
Copy link
Member

I guess you are right, I forgot about the need of passing image_base_name: &str, operator_version: &str to resolve.
So let's keep the API as you have introduced it ;)

I created a PR for a small refactoring, would be awesome if you could have a look! #818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants