-
Notifications
You must be signed in to change notification settings - Fork 7
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 by Bors] - TrinoClusters must specify a catalogLabelSelector #277
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.
LGTM. But breaking change without discussion?
We discussed this in Slack and Lars, Sönke and Teo approved (send you the link) |
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.
LGTM but please wait for the tests!
Tests succeeded |
bors r+ |
# Description During bumping the demos to the 22.09 i noticed that the following spec from the Trino tests and docs is invalid ``` apiVersion: trino.stackable.tech/v1alpha1 kind: TrinoCluster metadata: name: simple-trino spec: version: 387-stackable0.1.0 catalogLabelSelector: trino: simple-trino ``` instead it should be ``` # ... catalogLabelSelector: matchLabels: trino: simple-trino ``` Nobody noticed this, as the field `catalogLabelSelector` is optional. If it is not provided, the (nonexistent) Labelselector will match every TrinoCatalog withing the same Namespace and the test passed anyway. This behavior sounds unsafe/unwanted so i suggest to make it explicit by requiring a Labelselector. Although this change is technically breaking, the `TrinoCatalog` feature change has been release for a single day now, so impact should be pretty minimal. It would be greatif we could include this Change in 22.09, so that we reduce the number of breaking changes between Platform releases ## Review Checklist - [x] Code contains useful comments - [x] CRD change approved (or not applicable) - [x] (Integration-)Test cases added (or not applicable) - [x] Documentation added (or not applicable) - [x] Changelog updated (or not applicable) - [x] Cargo.toml only contains references to git tags (not specific commits or branches) - [x] Helm chart can be installed and deployed operator works (or not applicable) Once the review is done, comment `bors r+` (or `bors merge`) to merge. [Further information](https://bors.tech/documentation/getting-started/#reviewing-pull-requests)
Pull request successfully merged into main. Build succeeded: |
Description
During bumping the demos to the 22.09 i noticed that the following spec from the Trino tests and docs is invalid
instead it should be
Nobody noticed this, as the field
catalogLabelSelector
is optional. If it is not provided, the (nonexistent) Labelselector will match every TrinoCatalog withing the same Namespace and the test passed anyway.This behavior sounds unsafe/unwanted so i suggest to make it explicit by requiring a Labelselector.
Although this change is technically breaking, the
TrinoCatalog
feature change has been release for a single day now, so impact should be pretty minimal. It would be greatif we could include this Change in 22.09, so that we reduce the number of breaking changes between Platform releasesReview Checklist
Once the review is done, comment
bors r+
(orbors merge
) to merge. Further information