Fix installation of helm charts #5841
Conversation
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
…nents to be rendered in unit tests Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
jansav
requested review from
nevalla and
Iku-turso
and removed request for
a team
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
|
|
||
| return { | ||
| value: computed( | ||
| () => state.get() || versionsOfSelectedHelmChart.value.get()[0], |
There was a problem hiding this comment.
nit, using ?? is more semantically correct.
| @@ -59,7 +61,7 @@ const NonInjectedEditorPanel = observer(({ | |||
| autoFocus={autoFocus} | |||
| id={tabId} | |||
| value={value} | |||
| className={cssNames(styles.EditorPanel, className)} | |||
| className={cssNames(styles.EditorPanel, className, { hidden })} | |||
There was a problem hiding this comment.
This seems strange, why is this needed?
There was a problem hiding this comment.
Because MonacoEditor works incorrectly
const SomeComponent = observer(({ someProp, value }: Props) => (
<div>
{someProp.get() ? <MonacoEditor value={value.get()} /> : <div>Something else</div>}
</div>
));someProp.get()returns truthy value,MonacoEditoris renderedsomeProp.get()changes to falsy value,MonacoEditoris not renderedvalue.get()changessomeProp.get()changes back to truthy value,MonacoEditorrenders with old value
There was a problem hiding this comment.
Then shouldn't we fix MonacoEditor? That sounds like it isn't checking for an updated value prop (in componentDidUpdate or something).
There was a problem hiding this comment.
Yes, we should. Draw the line here, because there is lot to improve in MonacoEditor. Issue is that it's calling this.restoreViewState(this.model); when it creates the editor. Didn't go any deeper than that.
src/renderer/components/dock/install-chart/chart-data/call-for-helm-chart-values.injectable.ts
Show resolved
Hide resolved
| const getRandomInstallChartTabIdInjectable = getInjectable({ | ||
| id: "get-random-install-chart-tab-id", | ||
| instantiate: (di) => di.inject(getRandomIdInjectable), | ||
| }); | ||
|
|
There was a problem hiding this comment.
This is weird, why have this additional level of abstraction?
There was a problem hiding this comment.
Overriding in test. If I would have overridden getRandomIdInjectable then all of use places (even the ones not relevant here) would be overridden.
src/renderer/components/dock/install-chart/install-chart-model.injectable.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| instantiate: async (di, tabId: string) => { | ||
| const store = di.inject(installChartTabStoreInjectable); | ||
| const callForHelmChartValues = di.inject(callForHelmChartValuesInjectable); |
There was a problem hiding this comment.
comment: what is the benefit of naming these sort of injectables with the callFor prefix? Wouldn't getHelmChartValues be descriptive enough?
There was a problem hiding this comment.
It's just matter of preference and what we're used to.
We've been using callForX for stuff that needs to be get from somewhere outside of the system. E.g. API call, file system, local storage. getX doesn't really communicate enough because it could also mean that we are getting some static value from somewhere or e.g. some deeply nested value from the input.
| @@ -111,6 +112,9 @@ export const getDiForUnitTesting = (opts: { doGeneralOverrides?: boolean } = {}) | |||
|
|
|||
| di.override(historyInjectable, () => createMemoryHistory()); | |||
| di.override(legacyOnChannelListenInjectable, () => () => noop); | |||
|
|
|||
| di.override(storageSaveDelayInjectable, () => 0); | |||
There was a problem hiding this comment.
Why is this override different than that of main's getDiForUnitTesting?
There was a problem hiding this comment.
This is used only by createStorage which is renderer thing.
There was a problem hiding this comment.
Then why should main even know about it?
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>
Screen.Recording.2022-07-19.at.8.55.31.AM.movOr maybe this is related to the Upgrade bug |
Yes, this is related to the other issue I created today. #5852 |
|
The release details page slides in a few extra times (sometimes it doesn't happen till ~30 seconds later) Screen.Recording.2022-07-19.at.10.41.21.PM.mov |
|
crash after removing a release with an upgrade tab open Screen.Recording.2022-07-19.at.10.59.48.PM.mov |
Fixes #5827
Notice the added behaviours, they describe completely how it should work. There's still lot to improve in the implementation, but that's enough for now.