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.

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

Fix wrong namespace when requesting chart info #2081

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Conversation

andresmgot
Copy link
Contributor

Description of the change

Fix an issue causing the request for the chart to use the current namespace (_all if all namespaces is selected) rather than the app namespace.

Apart from that, I found a small bug that two releases in different namespaces were using the same key in the list.

Applicable issues

@@ -244,7 +244,7 @@ export function fetchAppsWithUpdateInfo(
dispatch(
getAppUpdateInfo(
cluster,
namespace,
app.namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of things confuse the code here, imo:

  • the all function argument is actually allStatuses not all apps across namespaces - can we rename it to be explicit (it's always passed as true from the only call-site afaict, so we could even get rid of it).
  • the fetchApps and subsequent App.listApps have functionality to support fetching apps across all namespaces which is the cause of this change, but it'd be a lot easier to reason about this code if it was obvious why namespace can't be used here (because it could have the value _all. Perhaps a comment, or renaming it namespace_or_all or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • the all function argument is actually allStatuses not all apps across namespaces - can we rename it to be explicit (it's always passed as true from the only call-site afaict, so we could even get rid of it).

Yes, since we have deleted the code for the Hex UI we can delete it.

  • the fetchApps and subsequent App.listApps have functionality to support fetching apps across all namespaces which is the cause of this change, but it'd be a lot easier to reason about this code if it was obvious why namespace can't be used here (because it could have the value _all. Perhaps a comment, or renaming it namespace_or_all or something.

👍

@andresmgot andresmgot merged commit 84e10f0 into master Sep 30, 2020
@andresmgot andresmgot deleted the appListAllNamespaces branch November 26, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chart new version indicator when select all namespaces
2 participants