fix: Chart: Allow percentage on PDBs#4852
fix: Chart: Allow percentage on PDBs#4852Skarlso merged 4 commits intoexternal-secrets:mainfrom achetronic:fix/allow-percentage-pdbs
Conversation
Signed-off-by: Alby Hernández <donfumero@gmail.com>
|
This will definitely break existing charts that use a number. 🤔 we have to be careful about this being released. |
Agree with you, I think we need a big advice on this, but IMHO, feature is needed :) |
|
Could you please update the tests? |
|
Okay, so since existing deployments shouldn't be affected by this ( because the helm upgrade would just fail until values are updated ) we should be okay to release this with a minor version increase. |
This is the reason i did not those changes on the PR :) |
|
they are still needed. :) I mean run |
Done, the branch is clean now. Thank you for the advise! |
|
Great! Thank you. Now all you need to do is sign your last commit as well. :D And we're done. :) |
Signed-off-by: Alby Hernández <donfumero@gmail.com>
|
Aaaaaand done! :) |
|
@gusfcarvalho Are you okay to merge this? It will break helm upgrades. But there is no way around that. podDistributionBudget can indeed be defined as something like |
|
This is just changing the defaults - from a buggy one. I thinks that's fine. |
|
we are already planning the next release to be |
|
just labeled it so we can filter it down 😄 |
|
What do you mean it's changing the defaults? It's literally changing the json schema. |
|
Although it is changing the schema, it’s not changing how yaml will process it for kubernetes afterwards (the simple fact that minAvailable: 1 worked even though we marked it as an integer is the proof of it). The only breaking change IMO would be people consuming from the schemas, no? 🙂 |
|
Furthermore, our policy for breaking changes does not include helm charts 😜😜 |
I appologice for the drive by comment, but you can configure JSON Schemas to allow 2 different Types for a property1, so in theory, you could allow both. i.e. The following allows for both Integers and strings as valid input. "minAvailable": {
"type": ["integer", "string"]
}The only thing that might be a blocker is any generation scripts used to update the readme or other bits. Footnotes |
|
If a schema file is defined that file will be used to validate the values file for any upgrade install template and lint commands. That's why there has to be a change in our values yaml file as well. Helm lint was failing. Meaning if we release this it will break the helm upgrade command people are running in a ci. The opt out is to run it with disable validation or something like that.
If the schemas file is there you use it automatically. Anyways. I like that you can define both. That could work better. WDYT @achetronic? |
yeah, that’s why this would be a breaking change. I’m not saying it isn’t going to be, just that the impact is limited to quote whatever the values users bring that are not compliant. I wouldn’t see a problem with it on a minor release, that was my point. in any case the double type validation is a much better approach 🙂🙂 |
|
@gusfcarvalho Oh okay, gotcha. Sorry, I misunderstood you. 🙇 |
|
Thanks :) |
|
Ah sorry, you need to run |
| enabled: false | ||
| minAvailable: 1 | ||
| # maxUnavailable: 1 | ||
| minAvailable: 1 # @schema type:[integer, string] |
There was a problem hiding this comment.
try putting this on the TOP of this line. Then it should be okay.
There was a problem hiding this comment.
@Skarlso Hello there :) those annotations only works when they are in the side. When putting them on top, not working at all
There was a problem hiding this comment.
Ugh, thought so. Was worth a try. Not sure what to do about the linter. Maybe just add a space or something? Can you fiddle with what the linter wants locally?
There was a problem hiding this comment.
Because of fixing this and TOCs i added exactly 4 spaces :) is it fine?
There was a problem hiding this comment.
Fine for me if the linter doesn't complain and it's still working. :)
There was a problem hiding this comment.
unit tests failing, but I think it doesn't matter for the purpose of this changes as no merging code, right? :)
Signed-off-by: Alby Hernández <donfumero@gmail.com>
|
Ya, I "fixed" the unit test. Sorry about that. |
|
Thank you! I assume this will be released in v0.17.1, right? |
|
0.18.0. :) Not because of this most likely, but we have other things in the pipeline I believe that might be breaking changes. |
|
Regardless, whatever comes next, this will be in it. :D |
* fix: Chart: Allow percentage on PDBs Signed-off-by: Alby Hernández <donfumero@gmail.com> * fix: populate changes Signed-off-by: Alby Hernández <donfumero@gmail.com> * fix: Use double type validation for chart values Signed-off-by: Alby Hernández <donfumero@gmail.com> --------- Signed-off-by: Alby Hernández <donfumero@gmail.com> Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Signed-off-by: Pedro Parra Ortega <pedro.parraortega@enreach.com>
* fix: Chart: Allow percentage on PDBs Signed-off-by: Alby Hernández <donfumero@gmail.com> * fix: populate changes Signed-off-by: Alby Hernández <donfumero@gmail.com> * fix: Use double type validation for chart values Signed-off-by: Alby Hernández <donfumero@gmail.com> --------- Signed-off-by: Alby Hernández <donfumero@gmail.com> Co-authored-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> Signed-off-by: asrormirzoev <asrormirzoev@yandex-team.ru>
Problem Statement
Chart is not allowing percentages on PodDisruptionBudget objects, which is in fact, the recommended way to express budgets
Related Issue
N/A
Proposed Changes
Instead of checking the type being an integer, with a string it's enough for both cases as chart is setting the value directly without any additional check
Checklist
git commit --signoffmake testmake reviewable