fix hardcoded references to kubeapps namespace in dashboard #425

prydonius · 2018-08-02T23:55:02Z

also moves config into the redux store

fixes #415

…I calls

migmartri · 2018-08-03T00:03:22Z

dashboard/src/shared/App.ts

    chartVersion: IChartVersion,
    values?: string,
  ) {
    const chartAttrs = chartVersion.relationships.chart.data;
-    const repo = await AppRepository.get(chartAttrs.repo.name);
+    const repo = await AppRepository.get(chartAttrs.repo.name, kubeappsNamespace);
    const auth = repo.spec.auth;


Not from this patch but just pointing out inconsistent usage of es6 destructuring.

const { auth } = repo.spec;

Should I fix this in this PR?

migmartri · 2018-08-03T00:05:05Z

dashboard/src/shared/AppRepository.ts

 import { IAppRepository, IAppRepositoryList } from "./types";

 export class AppRepository {
-  public static async list() {
-    const { data } = await axios.get<IAppRepositoryList>(await AppRepository.getResourceLink());
+  public static async list(namespace: string) {


is this namespace a kubeappsNamespace?

yes, but I don't think AppRepository should really know about that, it just cares about what namespace to request from in the API

Agreed, thanks for the explanation.

migmartri · 2018-08-03T00:05:40Z

dashboard/src/shared/AppRepository.ts

-    return `${AppRepository.APIEndpoint}/namespaces/${namespace}/apprepositories${
-      name ? `/${name}` : ""
-    }`;
+  private static getResourceLink(namespace?: string): string {


Could you please add a comment on why this URI resolution is needed?

We're doing this in other classes so it's a common model and not something specific for this class

migmartri · 2018-08-03T00:07:47Z

dashboard/src/shared/App.ts

@@ -19,11 +19,12 @@ export class App {
  public static async create(
    releaseName: string,


So this shared directory is like a shared set of models?

Yes, correct. We try to keep all the API related methods here and not in the actual actions.

migmartri · 2018-08-03T00:08:13Z

dashboard/src/shared/types.ts

@@ -3,6 +3,7 @@ import { IServiceCatalogState } from "../reducers/catalog";
 import { IFunctionState } from "../reducers/functions";
 import { INamespaceState } from "../reducers/namespace";
 import { IAppRepositoryState } from "../reducers/repos";
+import { IConfig } from "./Config";


What's the reason for the I prefix?

enforced by tslint https://palantir.github.io/tslint/rules/interface-name/

migmartri · 2018-08-03T00:21:42Z

dashboard/src/shared/Config.ts

@@ -1,20 +1,15 @@
 import axios from "axios";

-interface IConfig {
+export interface IConfig {


can you add a comment saying that this is the kubeapps configuration?

migmartri · 2018-08-03T00:22:06Z

dashboard/src/shared/Config.ts

  namespace: string;
 }

 export default class Config {
  public static async getConfig() {
-    if (Config.config) {
-      return Config.config;
-    }
    const url = `${Config.APIEndpoint}`;


do you need the backticks?

👍 will fix

migmartri · 2018-08-03T00:25:32Z

dashboard/src/containers/Root.tsx

@@ -14,6 +14,7 @@ import AppView from "./AppViewContainer";
 import ChartList from "./ChartListContainer";
 import ChartView from "./ChartViewContainer";
 import ClassListContainer from "./ClassListContainer";
+import { ClassViewContainer } from "./ClassView";


I moved this for consistency, the package imports are separated from local imports

migmartri · 2018-08-03T00:26:19Z

dashboard/src/reducers/config.ts

+import { IConfig } from "../shared/Config";
+
+const initialState: IConfig = {
+  namespace: "default",


do we really want a default? I am wondering if this could have side-effects in the case that there are problems while retrieving the value from the config endpoint.

It's possible that there's a race condition here, yes. I need to look into this more.

fixed, PTAL @migmartri

andresmgot

an alternative solution would be to add an env var in the dashboard container with the namespace of the pod:

        - name: KUBEAPPS_NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace

I think it will be easier to read an env var (it's synchronous) rather than getting the namespace from a configmap (that can be misconfigured).

That wouldn't work if the dashboard is in a different namespace than the rest of Kubeapps but I don't think that's a possibility, isn't it?

andresmgot · 2018-08-03T07:42:19Z

dashboard/src/components/UpgradeForm/SelectRepoForm.tsx

-              namespace={RequiredRBACRoles.namespace || ""}
-              roles={[RequiredRBACRoles]}
-              action={`list app repositories`}
+              namespace={this.requiredRBACRoles().namespace || ""}


namespace={this.props.namespace} is a bit more simple

andresmgot · 2018-08-03T08:06:37Z

dashboard/src/shared/Config.ts

  }

-  private static config: IConfig;
  private static APIEndpoint: string = "/config.json";


It's not related to this PR but this is a bit confusing (using axios.get with an "URL" to read a local file). Mostly because this method is now unnecessary async.

@andresmgot perhaps you're misunderstanding this a bit? The nginx server serves the config.json file, it's not embedded in the React application. I'm not sure what you mean by local file, at some point the React app has to fetch this from the server.

I think that the key here is that the React app is just a bundle of static files so AFAIK there is no way of exposing this env variable without triggering a build (via webpack) at deployment time.

If there was a way for Nginx to expose this env variable as a header that could be an alternative. Although I have not found any example nor insights on how to achieve so.

true, completely misunderstood from my side. Sorry!

No problem, thanks for asking!

prydonius · 2018-08-03T20:40:15Z

an alternative solution would be to add an env var in the dashboard container with the namespace of the pod

Hmm I'm not sure how we'd read the env var and respond with that from the nginx server, we could use an initContainer that creates the config.json file using that env var, but that might make it more difficult to add more configuration later. @migmartri what do you think?

prydonius · 2018-08-04T01:13:26Z

@migmartri @andresmgot review addressed, PTAL!

andresmgot

we could use an initContainer that creates the config.json file using that env var, but that might make it more difficult to add more configuration later

Agree, nvm. LGTM right now.

migmartri

Thanks for the changes.

migmartri · 2018-08-06T17:12:14Z

dashboard/src/containers/Root.tsx

-            ))}
-          </Layout>
-        </ConnectedRouter>
+        <ConfigLoaderContainer>


migmartri · 2018-08-06T17:14:23Z

dashboard/src/reducers/config.ts

+  switch (action.type) {
+    case getType(actions.config.requestConfig):
+      return initialState;
+    case getType(actions.config.receiveConfig):


Minor comment. Shouldn't this be receivedConfig instead of receiveConfig? Just feels like receiveConfig is setting the intention instead of the result.

migmartri · 2018-08-06T17:15:48Z

dashboard/src/reducers/config.ts

+
+const configReducer = (state: IConfigState = initialState, action: ConfigAction): IConfigState => {
+  switch (action.type) {
+    case getType(actions.config.requestConfig):


Not for this patch but in general we should start thinking of error handling so we do not run into cases like this one #433

migmartri · 2018-08-06T17:16:07Z

dashboard/src/shared/App.ts

    chartVersion: IChartVersion,
    values?: string,
  ) {
    const chartAttrs = chartVersion.relationships.chart.data;
-    const repo = await AppRepository.get(chartAttrs.repo.name);
+    const repo = await AppRepository.get(chartAttrs.repo.name, kubeappsNamespace);
    const auth = repo.spec.auth;


migmartri · 2018-08-06T17:16:30Z

dashboard/src/shared/AppRepository.ts

 import { IAppRepository, IAppRepositoryList } from "./types";

 export class AppRepository {
-  public static async list() {
-    const { data } = await axios.get<IAppRepositoryList>(await AppRepository.getResourceLink());
+  public static async list(namespace: string) {


Agreed, thanks for the explanation.

…anzu#425) * add config to redux store * use kubeapps install namespace from redux config store for AppRepo API calls * ensure config is loaded before loading app * comment for IConfig interface * remove unnecessary string interpolation * use kubeappsNamespace prop directly

Adnan Abdulhussein added 2 commits August 2, 2018 16:54

add config to redux store

730c016

use kubeapps install namespace from redux config store for AppRepo AP…

025e3d2

…I calls

prydonius changed the title ~~fixes #415~~ fix hardcoded references to kubeapps namespace in dashboard Aug 2, 2018

prydonius requested review from migmartri and andresmgot August 2, 2018 23:56

prydonius added the component/ui Issue related to kubeapps UI label Aug 2, 2018

migmartri approved these changes Aug 3, 2018

View reviewed changes

andresmgot reviewed Aug 3, 2018

View reviewed changes

Adnan Abdulhussein added 4 commits August 3, 2018 18:09

ensure config is loaded before loading app

6a16e18

comment for IConfig interface

8617958

remove unnecessary string interpolation

cb6a9ed

use kubeappsNamespace prop directly

5c34906

andresmgot approved these changes Aug 6, 2018

View reviewed changes

migmartri approved these changes Aug 6, 2018

View reviewed changes

prydonius merged commit b86b8d7 into vmware-tanzu:master Aug 6, 2018

prydonius deleted the 415-fix-hardcoded-references branch August 6, 2018 22:35

		@@ -19,11 +19,12 @@ export class App {
		public static async create(
		releaseName: string,

fix hardcoded references to kubeapps namespace in dashboard #425

fix hardcoded references to kubeapps namespace in dashboard #425

Conversation

prydonius commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Aug 3, 2018

prydonius commented Aug 4, 2018

andresmgot left a comment

Choose a reason for hiding this comment

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius commented Aug 2, 2018 •

edited

Loading

prydonius Aug 3, 2018 •

edited

Loading

andresmgot Aug 3, 2018 •

edited

Loading