-
Notifications
You must be signed in to change notification settings - Fork 151
[CC-33024] Remove the deprecated fields of CockroachDB operator #560
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
base: master
Are you sure you want to change the base?
Conversation
13ac257 to
b01e115
Compare
| clusterSettings: ~ | ||
| # timestamp captures the annotation timestamp used for rolling restarts. | ||
| timestamp: "2021-10-18T00:00:00Z" | ||
| # resources captures the resource requests and limits for CockroachDB pods. |
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.
I believe we need to remove the whole resources section here.
build/templates/cockroachdb-parent/charts/cockroachdb/values.yaml
Outdated
Show resolved
Hide resolved
0f4e15d to
1238344
Compare
1238344 to
f886c28
Compare
| # If specified, podTemplate is merged with the default pod specification, with settings in podTemplate taking precedence. | ||
| # This can be used to add or update containers, volumes, and other settings of the CockroachDB pod. | ||
| podTemplate: {} | ||
| podTemplate: |
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.
nit: need to fix the indentation (of comments) within the podTemplate section.
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.
Done
162b3a6 to
647a3a0
Compare
pritesh-lahoti
left a comment
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.
@prafull01 - as discussed, let's add a changelog entry for these changes and seek @jhlodin 's review for it.
647a3a0 to
3ab1dae
Compare
CHANGELOG.md
Outdated
| All notable changes to this project will be documented in this file. | ||
|
|
||
| ## [cockroachdb-parent-25.3.3-preview+1] 2025-10-29 | ||
| - Remove the deprecated fields and start using the podTemplate fields. |
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.
We should list the deprecated fields, at least at a high level. Like:
- Removed the following deprecated fields in favor of `podTemplate`:
- `crdbCluster.resources.*`
- `crdbCluster.podLabels.*`
- `crdbCluster.topologySpreadConstraints.*`
etc.
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.
That'll also make it easier for me to identify which parts of the docs need to be updated to remove reference to deprecated, so if for some reason we don't want to list this out in the release note I could still use that.
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.
I understand to know before hand which fields are deprecated. However this change would make the CHANGELOG.md very log. Should we add that description here or I can add that in the ticket so it would be easier for you to write the documentation. This will add additional 10-12 line specific to deprecation in CHANGELOG.
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.
Personally I think 10-12 lines is acceptable. I've put similar lists in breaking changes/release notes in the past and AFAIK it was received well - people appreciate having the detail when you introduce a breaking change like deprecation/removal, and if they need to ctrl+F a field it's useful having it in the list. Best if we can have sub-bullets for each field like I proposed in my first comment, rather than individual release notes for every deprecation.
3ab1dae to
e0b0875
Compare
1c7e18c to
7403e57
Compare
jhlodin
left a comment
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.
One quick suggestion, otherwise LGTM
CHANGELOG.md
Outdated
| All notable changes to this project will be documented in this file. | ||
|
|
||
| ## [cockroachdb-parent-25.3.3-preview+1] 2025-10-29 | ||
| - Remove the deprecated fields and start using the podTemplate fields. Following fields are deprecated: |
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.
| - Remove the deprecated fields and start using the podTemplate fields. Following fields are deprecated: | |
| - Remove the following deprecated fields in favor of the corresponding podTemplate fields: |
7403e57 to
e2c081c
Compare
| # disablethp determines whether Transparent Huge Pages are disabled. | ||
| # By default, disables THP, which can cause memory inefficiency for CockroachDB. | ||
| disablethp: "1" | ||
| # podTemplate is an optional pod specification that overrides the default pod specification configured by the operator. |
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.
Do we need to rewrite this description? Seems like it's not actually optional since now it contains the default pod specifications.
No description provided.