Merge modifications made in previous versions when upgrading #1278
Conversation
| />, | ||
| ); | ||
| // Store the modifications | ||
| wrapper.setProps({ selected: props.selected }); |
There was a problem hiding this comment.
I don't understand why we have a third type of selected here - props.selected, nor why we need to explicitly change props to trigger storing the modification when it could be stored on create.
It looks like it might be a work around because we're not storing the modifications when the DeploymentFormBody is initialized, but should be? That is, when upgrading, I would think that the DeploymentFormBody should have access to:
- The default values of the deployed chart version
- The actual values of the deployed chart version
- The default values of the latest (
selected) chart version
With that info, the modifications could be calculated when the form is first created, once - without needing conditionals in the componentWillReceiveProps etc? The only thing which would need updating there is the application of the modifications to the selected chart version?
Things like this are, imo, worth the extra initial effort to do correctly, as they cause every person who reads the code later (not just me reviewing) to ask the same questions and spend effort trying to understand why? Or have I missed something?
There was a problem hiding this comment.
sorry, these two lines are actually unnecessary since the DeploymentFormBody initialization with the selected chart there already executes componentWillReceiveProps.
The issue is that with this code is really being executed, the selected property is undefined, it's not until the component executes the componentDidMount and therefore getChartVersion when the selected chart is requested and eventually populated.
There was a problem hiding this comment.
Sorry, it was actually needed (I was executing the wrong tests). I have adapted them to reflect reality in the most similar way... But maybe you are right, we may need to request the default chart version before and pass it to this component even though I would prefer to do so in a different PR. Let me know what you think.
| const defaultValuesObj = YAML.parse(nextProps.selected.values); | ||
| const deployedValuesObj = YAML.parse(this.props.deployedValues); | ||
| modifications = jsonpatch.compare(defaultValuesObj, deployedValuesObj); | ||
| this.setState({ modifications }); |
There was a problem hiding this comment.
It looks like you're calculating the modifications based on the new selected chart and the default values... which explains why in the test you're setting the modifications by first selecting the original chart (props.selected) and basing it on that.
This is quite confusing - as above, it's not clear why we don't calculate the modifications once when the component is first created, rather than wrapping the calculation of the modifications in multiple conditions here to ensure it only gets calculated once? Yes, it might be a little more work initially to support that, but in the long run it will save orders of magnitude more person-hours as we review and work on this code - IMO.
There was a problem hiding this comment.
Sorry for the confusion in the test because it's not properly written but as I said, when the component is first created (in the real scenario) the selected property is empty so we cannot get the modifications at that moment. It's not until the code in the line 55 is executed when we have that information available. That's why I wrap this in the condition. This hook will be executed every time the user selects a new version but we are only interested on calculating the modifications the first time since at that moment is when the version matches the already deployed chart.
In any case, I plan to change this once we change the UX experience to pre-select the latest version available, I just didn't want to add more changes to this PR (also because I am not sure yet how that will look like).
There was a problem hiding this comment.
Sorry for the confusion in the test because it's not properly written
Thanks for the update.
but as I said, when the component is first created (in the real scenario) the
selectedproperty is empty so we cannot get the modifications at that moment.
Right, but that's because of confusion (in the implementation) on the selected property I think? That is, we should not be calculating any user modifications on the current deployment based on the selected field - it's only working here because you're passing through the currently deployed chart as the selected version initially (hence the confusion in the code).
Sorry to be pushing back, but I really think stuff like this is important... I get that it all makes sense to you currently in your head, with the context, but it won't be clear to anyone else (and not you either at a later date). It'd be slightly better if there was a chunk of comment explaining why it's being done the way it is, but afaict, but better to avoid the confusion in the code itself.
My main question is: Why are we mis-using the selected prop of the DeploymentFormBody, which is for which chart version a user selects in the UI - not which version is already deployed, rather than required prop on the UpgradeForm? (when the user is upgrading, this info should be required to start the upgrade process).
In any case, I plan to change this once we change the UX experience to pre-select the latest version available, I just didn't want to add more changes to this PR (also because I am not sure yet how that will look like).
I don't see how that's related at all - we don't need to know the version the user is selecting (or the latest version) to be able to calculate the modifications for the currently deployed chart?
Let's chat about this on the standup (or after). I don't want to block you further, but do want to understand why we're mis-using props like this (or what I'm misunderstanding) before approving.
There was a problem hiding this comment.
Thanks Andres. FTR, we chatted and found one piece of info which wasn't clear to me is that in the current implementation, the selected field is updated to display the value of the currently deployed version when you upgrade (which we'd want to change to instead default to the most likely upgrade candidate - ie. the most recent version).
Andres is going to look at doing this (possibly in another PR), so that we don't need to abuse and depend on the selected field to record the currently deployed chart, perhaps extracting to the UpgradeForm.
There was a problem hiding this comment.
right, it's also true that it may not necessary to control the behavior of the DeploymentFormBody depending if it's for the Upgrade or the Deployment case.
Ref #1235
After this PR, changes made to the default values of a chart will be migrated to the new version. This is the process followed:
DeploymentFormBody, we calculate the difference between the default values of the current chart and the current values of the release. We store that diff as a JSON Patch in the state.Note: I have renamed the property
originalValueswithdeployedValueshoping it's more readable. This is the variable that contains the currently installed values.Note 2: We maintain the previous behavior regarding manual changes: Once the user changes anything in the values, we don't modify it anymore.