add required flag for upgrade to ack full reingest but not on install #3410
Conversation
|
Opening this up for discussion ASAP. I am very open to better wording and a more user friendly way to do this, if there are ideas. I would prefer to keep track of these migrations so we can be smarter about this - something along the lines of storing the data in a configmap. |
| {{- if semverCompare ">= 2.1.0-0 <2.4.0-0" .Chart.Version }} | ||
| {{- if gt .Release.Revision 1 -}} | ||
| {{- if not .Values.upgrade.acknowledgeV2dot3FullReingestion -}} | ||
| {{- fail "\n\nKubecost 2.3 requires a full reingestion of all data which will could take significant time. Please acknowledge by setting 'upgrade.acknowledgeV2dot3FullReingestion' to 'true' in your values file." -}} |
There was a problem hiding this comment.
What if we have the value be upgrade.verifyVersion=2.3 and then if we need to use it in the future we use the same field and just increment the value?
There was a problem hiding this comment.
We can totally go this route. My concerns with that path are:
- It is significantly less clear to the customer when they can remove the value from their values file
- To someone looking at the file 6 months down the road - it is a lot harder to understand what this is, other than something they added in to make the errors go away. A more specific name -
lastAcknowledgedFullReingestionVersion=2.3could help here.
There was a problem hiding this comment.
May want to consider the logic here if you haven't already.
| {{- if semverCompare ">= 2.1.0-0 <2.4.0-0" .Chart.Version }} | ||
| {{- if gt .Release.Revision 1 -}} | ||
| {{- if not .Values.upgrade.acknowledgeV2dot3FullReingestion -}} | ||
| {{- fail "\n\nKubecost 2.3 requires a full reingestion of all data which will could take significant time. Please acknowledge by setting 'upgrade.acknowledgeV2dot3FullReingestion' to 'true' in your values file." -}} |
There was a problem hiding this comment.
May want to consider the logic here if you haven't already.
| @@ -304,6 +304,9 @@ global: | |||
| ## | |||
| upgrade: | |||
| toV2: false | |||
| ## This flag is only required for users upgrading to 2.3 or later from a previous version. | |||
| ## This is because 2.3 will trigger a full re-ingestion of all available data into aggregator. | |||
| acknowledgeV2dot3FullReingestion: false | |||
There was a problem hiding this comment.
I'd suggest making this value simpler. The whole matter of re-ingestion is an implementation detail. The user-facing point is just "get me to 2.3".
What does this PR change?
Users upgrading to a version through helm starting with 2.3.0 will be required to pass a helm value acknowledging a full re-ingestion may be triggered. Fresh installs will not require the flag (but on an additional helm revision to another 2.3.x version, will be required to pass the flag).
Does this PR rely on any other PRs?
No
How does this PR impact users? (This is the kind of thing that goes in release notes!)
Values.upgrade.acknowledgeV2dot3FullReingestion will be required for upgrading users
Links to Issues or tickets this PR addresses or fixes
n/a?
What risks are associated with merging this PR? What is required to fully test this PR?
Annoyance - this flag is currently fairly dumb - it will be required on any (non-installation) upgrade for 2.3 bugfixes, even though there is no full reingestion and db migration taking place.
How was this PR tested?
Manually
Have you made an update to documentation? If so, please provide the corresponding PR.
Not yet - this PR is just a fast way to collect feedback