Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(upgrade): Fetch package from its installed cluster #3525

Merged
merged 2 commits into from
Oct 5, 2021
Merged

fix(upgrade): Fetch package from its installed cluster #3525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb5a60d
fix(upgrade): Fetch package from its installed cluster
aanthonyrizzo Sep 29, 2021
7fec7dd
fix(cluster): Fetch correct cluster for deployed chart
aanthonyrizzo Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion dashboard/src/components/AppUpgrade/AppUpgrade.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ function AppUpgrade() {
dispatch(
actions.charts.getDeployedChartVersion(
{
context: { cluster: cluster, namespace: repoNamespace ?? "" },
context: {
cluster: app?.availablePackageRef?.context?.cluster ?? cluster,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Anthony. Yes, this should definitely be defaulting to the cluster of the available package, rather than the cluster of the installed app. +1 from me. Is the other discussion below resolved Antonio?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, everything is ready to go, I think. Indeed, given how messy the upgrade view was becoming I gave it a shot and try to perform some improvements (#3540).
However, the scope of the change is much bigger and I don't think we should release with my PR, but with this simpler and laser-focus one.

Let me jump in and merge the latest changes in the main branch (which hopefully should fix the CI tests...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antgamdia that PR is def gonna be a good improvement. There are a lot of rerenders that end up going all the way down the chain and they could be optimized and improve the performance of the page for sure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of rerenders that end up going all the way down the chain

Yeah... in fact we have been facing some CI issues because of that (the dropdown was being re-rendered multiple times making our e2e engine chose the wrong version)

Let me jump in and merge the latest changes in the main branch (which hopefully should fix the CI tests...)

I've tried but no permissions to edit this PR, I think. Please, @aanthonyrizzo, pull the latest changes in master and merge them into your branch. Let's see if tomorrow we can start tagging a release :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH. Sorry we changed the ownership of the repo from my user to our organization. I just rebased so should be up to date.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, perhaps it is why the CI is failing when building the images? Not sure, but maybe we should create another PR from this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that could be. I'm going to be away for a bit but feel free to cherry pick the commits into another branch and merge :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, don't know, re-triggering it made the trick. CI things...
Merging! Thanks again for the contributions :)

namespace: repoNamespace ?? "",
},
identifier: app?.availablePackageRef?.identifier ?? "",
plugin: app?.availablePackageRef?.plugin,
} as AvailablePackageReference,
Expand Down
10 changes: 8 additions & 2 deletions dashboard/src/components/UpgradeForm/UpgradeForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,10 @@ it("fetches the available versions", () => {
Chart.getAvailablePackageVersions = getAvailablePackageVersions;
mountWrapper(defaultStore, <UpgradeForm {...defaultProps} />);
expect(getAvailablePackageVersions).toHaveBeenCalledWith({
context: { cluster: defaultProps.cluster, namespace: defaultProps.repoNamespace },
context: {
cluster: defaultProps.cluster,
namespace: defaultProps.repoNamespace,
},
identifier: defaultProps.packageId,
plugin: defaultProps.plugin,
} as AvailablePackageReference);
Expand Down Expand Up @@ -193,7 +196,10 @@ it("fetches the current chart version even if there is already one in the state"
);
expect(getAvailablePackageDetail).toHaveBeenCalledWith(
{
context: { cluster: defaultProps.cluster, namespace: defaultProps.repoNamespace },
context: {
cluster: availablePkgDetails[0].availablePackageRef?.context?.cluster,
namespace: defaultProps.repoNamespace,
},
identifier: defaultProps.packageId,
plugin: defaultProps.plugin,
} as AvailablePackageReference,
Expand Down
15 changes: 10 additions & 5 deletions dashboard/src/components/UpgradeForm/UpgradeForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ function UpgradeForm({

const { availablePackageDetail, versions, schema, values, pkgVersion } = selected;

const packageCluster = availablePackageDetail?.availablePackageRef?.context?.cluster;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a child component depending upon the AppUpgrade view, I wonder whether we should get perform the changes in the view instead.

Given that a UpgradeForm will always be rendered if the app is defined, why not reducing the number of attributes being passed to this component?

But, perhaps we can just add this simple change and later on, review all the namespace/cluster being passed through. (I've added a note in our project board for having a discussion about it)

Also, the tests are failing, I think (not the e2e, but the dashboard_test).

As usual, thanks a lot for contributing to Kubeapps!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay interesting, yeah maybe putting it in the parent component would be best. I can give it another one over when I fix the tests.


const {
apps: { isFetching: appsFetching },
charts: { isFetching: chartsFetching },
Expand All @@ -91,12 +93,15 @@ function UpgradeForm({
useEffect(() => {
dispatch(
actions.charts.fetchChartVersions({
context: { cluster: cluster, namespace: repoNamespace },
context: {
cluster: packageCluster ?? cluster,
namespace: repoNamespace,
},
plugin: pluginObj,
identifier: packageId,
} as AvailablePackageReference),
);
}, [dispatch, cluster, repoNamespace, packageId, pluginObj]);
}, [dispatch, packageCluster, repoNamespace, packageId, cluster, pluginObj]);

useEffect(() => {
if (deployed.values && !modifications) {
Expand All @@ -122,7 +127,7 @@ function UpgradeForm({
dispatch(
actions.charts.fetchChartVersion(
{
context: { cluster: cluster, namespace: repoNamespace },
context: { cluster: packageCluster, namespace: repoNamespace },
plugin: pluginObj,
identifier: packageId,
} as AvailablePackageReference,
Expand All @@ -131,7 +136,7 @@ function UpgradeForm({
);
}, [
dispatch,
cluster,
packageCluster,
repoNamespace,
packageId,
deployed.chartVersion?.version?.pkgVersion,
Expand Down Expand Up @@ -160,7 +165,7 @@ function UpgradeForm({
dispatch(
actions.charts.fetchChartVersion(
{
context: { cluster: cluster, namespace: repoNamespace },
context: { cluster: packageCluster, namespace: repoNamespace },
plugin: pluginObj,
identifier: packageId,
} as AvailablePackageReference,
Expand Down