Add manifests #32

bharathappali · 2023-07-14T07:48:13Z

No description provided.

bharathappali · 2023-07-24T18:14:44Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2023-07-24T18:15:12Z

PR Analysis

🎯 Main theme: Adding manifests for deployment and environment variables
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
✨ Focused PR: Yes, the PR is focused as it is primarily concerned with adding manifests for deployment and setting up environment variables. However, the PR lacks a description which could provide more context about the changes.
🔒 Security concerns: No, the PR does not introduce any obvious security concerns. However, it's always a good practice to ensure that environment variables and configurations do not expose sensitive information.

PR Feedback

💡 General PR suggestions: The PR is generally well-structured and follows good practices. However, it would be beneficial to include a description for the PR to provide more context about the changes. Additionally, it would be good to consider adding tests to ensure the correctness of the new code.

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve: Suggest improvements to the code in the PR. These will be provided as pull request comments, ready to commit.
/ask <QUESTION>: Pose a question about the PR.

bharathappali · 2023-07-24T18:18:47Z

@CodiumAI-Agent /improve

CodiumAI-Agent · 2023-07-24T18:19:33Z

src/app/Utils/KruizeUIConstants.ts

+export const KRUIZE_API_URL = process.env.KRUIZE_APP_API_URL || undefined;
+export const KRUIZE_API_PORT = process.env.KRUIZE_APP_API_PORT || undefined;


Suggestion: It's better to provide a default value for the environment variables in case they are not set. This can prevent potential issues of these variables being undefined.

Suggested change

export const KRUIZE_API_URL = process.env.KRUIZE_APP_API_URL || undefined;

export const KRUIZE_API_PORT = process.env.KRUIZE_APP_API_PORT || undefined;

export const KRUIZE_API_URL = process.env.KRUIZE_APP_API_URL || 'http://localhost';

export const KRUIZE_API_PORT = process.env.KRUIZE_APP_API_PORT || '8080';

dinogun · 2023-08-08T07:14:26Z

Dockerfile.minikube.deploy

@@ -0,0 +1,28 @@
+FROM node:16 AS builder


Is this Dockerfile specific to minikube? If so why? If not please rename it

Renamed it, Thanks for the review

dinogun · 2023-08-08T07:16:55Z

deploy.sh

@@ -1,4 +1,17 @@
 #!/bin/bash
+
+export KRUIZE_UI_IMAGE="docker.io/bharathappali/kruize-ui:central-config"


This needs to be changed

updated to quay.io/kruize/kruize-ui:test

dinogun · 2023-08-08T07:17:14Z

deploy.sh

@@ -1,4 +1,17 @@
 #!/bin/bash
+
+export KRUIZE_UI_IMAGE="docker.io/bharathappali/kruize-ui:central-config"
+KRUIZE_UI_MANIFEST_FILE="kruize-ui-nginx-setup.yml"


Is this needed?

yes we use envsubst to create the yaml and this is the file we update in dynamic and static folders

dinogun · 2023-08-08T07:18:16Z

deploy.sh

+
+POD_NAME="kruize-ui-nginx-pod"
+SERVICE_NAME="kruize-ui-nginx-service"
+NAMESPACE="monitoring"


monitoring is only for minikube. For openshift, we are using openshift-tuning

We have -e option which will set the namespace based on deployment platform

dinogun · 2023-08-08T07:20:09Z

kruize-ui.yml

@@ -0,0 +1,42 @@
+apiVersion: apps/v1


This file might need to be deleted

dinogun · 2023-08-08T07:21:10Z

manifests/dynamic/kruize-ui-nginx-setup.yml

@@ -0,0 +1,68 @@
+apiVersion: v1


All yaml files need to use the suffix .yaml and not .yml in line with the convention that we are following in all the other repos

changed it to yaml

dinogun · 2023-08-08T07:24:58Z

manifests/dynamic/kruize-ui-nginx-setup.yml

+spec:
+  containers:
+    - name: kruize-ui-nginx-container
+      image: docker.io/bharathappali/kruize-ui:central-config


Please update this

updated to kruize ui images on quay

dinogun · 2023-08-08T07:25:22Z

manifests/static/kruize-ui-nginx-setup.yml

+spec:
+  containers:
+    - name: kruize-ui-nginx-container
+      image: bharathappali/kruize-ui:test


Needs to be updated

updated to kruize ui images on quay

dinogun · 2023-08-08T07:26:05Z

src/app/Analytics/SRE_Analytics/MonitoringTable.tsx

+  // const Context = useContext(nodeContext);
+  // const ip = Context['cluster'];
+  // const port = Context['autotune'];
+  //
+  // let k_url: string;
+  //
+  // if (ip) {
+  //   k_url = ip + ':' + port;
+  // } else {
+  //   k_url = 'kruize';
+  // }
+  // const list_experiments_url = 'http://' + k_url + '/listRecommendations';


Remove commented code

Removed the commented code

dinogun · 2023-08-08T07:26:19Z

src/app/Analytics/SRE_Analytics/RecommendationTables.tsx

+  // const Context = useContext(nodeContext);
+  // const ip = Context['cluster'];
+  // const port = Context['autotune'];
+  // let k_url: string;
+  //
+  // if (ip) {
+  //   k_url = ip + ':' + port;
+  // } else {
+  //   k_url = 'kruize';
+  // }
+  // const list_recommendations_url =
+  //   'http://' +
+  //   k_url +
+  //   '/listRecommendations?experiment_name=' +
+  //   sessionStorage.getItem('Experiment Name') +
+  //   '&latest=false';


Remove commented code

Removed the commented code

dinogun · 2023-08-08T07:26:37Z

src/app/Analytics/SRE_Analytics/UsecaseSelection.tsx

+  // const Context = useContext(nodeContext);
+  // const ip = Context['cluster'];
+  // const port = Context['autotune'];
+  // let k_url: string;
+  //
+  // if (ip) {
+  //   k_url = ip + ':' + port;
+  // } else {
+  //   k_url = 'kruize';
+  // }
+  //
+  // const list_recommendations_url =
+  //   'http://' + k_url + '/listRecommendations?experiment_name=' + props.SREdata.experiment_name + '&latest=false';
+  // const list_experiment_url = 'http://' + k_url + '/listExperiments';


Remove commented code

Removed the commented code

bhanvimenghani · 2023-08-08T09:35:32Z

src/app/CentralConfig.tsx

+    }
+}
+
+export { getRecommendationsURLWithParams, getListExperimentsURL, getHostname, getPort, getRecommendationsURL };


we follow the practice of adding an extra line towards the end of each file so that minus sign is not there

Added the extra line, Thanks for reviewing

bhanvimenghani · 2023-08-08T09:38:33Z

src/app/HorizontalNav/HorizontalNav.tsx

+  // const ip = Context['cluster'];
+  // const port = Context['autotune'];
+
+  console.log("hello");


can we remove this console log statement as it just prints hello

bhanvimenghani · 2023-08-08T09:40:35Z

src/app/HorizontalNav/HorizontalNav.tsx

+  // const Context = useContext(nodeContext);
+  // const ip = Context['cluster'];
+  // const port = Context['autotune'];


can we remove these comments as they are not relevant to the current setup

Yes I have removed them now, thanks for the pointer

bhanvimenghani · 2023-08-09T07:48:13Z

src/app/CentralConfig.tsx

+    if (isProduction) {
+        return window.location.port || (window.location.protocol === "https:" ? "443" : "80");
+    } else {
+        return process.env.AUTOTUNE_PORT;


Can we change the variable name AUTOTUNE_PORT to KRUIZE_PORT everywhere to make it consistent with the terminology used in the readme and everywhere else in ui

updated to KRUIZE_PORT

Clean up and update deployment files push scripts for demo Use non-root user nginx Update docker file update the k_url stuff Update Nginx port Update the config port Signed-off-by: bharathappali <abharath@redhat.com>

dinogun · 2023-08-11T06:49:38Z

Dockerfile.old

@@ -0,0 +1,40 @@
+FROM node:16 AS builder


Please delete this file .... dont need this for now

Deleted the old docker file

dinogun · 2023-08-11T06:51:13Z

manifests/dynamic/kruize-ui-nginx-setup.yaml

+spec:
+  containers:
+    - name: kruize-ui-nginx-container
+      image: docker.io/bharathappali/kruize-ui:central-config


Please add the right image here

These will be generated by the deploy script and I have left them unchanged after moving the kruize demos way of depployment. Will change it

Made the image as quay.io/kruize/kruize-ui:test

dinogun · 2023-08-11T06:51:33Z

manifests/static/kruize-ui-nginx-setup.yaml

+spec:
+  containers:
+    - name: kruize-ui-nginx-container
+      image: bharathappali/kruize-ui:test


Image should be from the kruize repo

Made the image as quay.io/kruize/kruize-ui:test

Signed-off-by: bharathappali <abharath@redhat.com>

dinogun

LGTM

bharathappali self-assigned this Jul 14, 2023

bharathappali marked this pull request as draft July 14, 2023 07:48

CodiumAI-Agent reviewed Jul 24, 2023

View reviewed changes

bharathappali force-pushed the add-manifests branch from 07271de to 575b9fa Compare August 7, 2023 11:17

bharathappali marked this pull request as ready for review August 8, 2023 05:54

bharathappali requested a review from bhanvimenghani August 8, 2023 06:00

bhanvimenghani requested a review from dinogun August 8, 2023 06:07

dinogun reviewed Aug 8, 2023

View reviewed changes

bhanvimenghani reviewed Aug 8, 2023

View reviewed changes

bhanvimenghani reviewed Aug 9, 2023

View reviewed changes

bharathappali force-pushed the add-manifests branch from 6934ff7 to d6e7fb3 Compare August 10, 2023 11:32

Add manifests

23313c7

Clean up and update deployment files push scripts for demo Use non-root user nginx Update docker file update the k_url stuff Update Nginx port Update the config port Signed-off-by: bharathappali <abharath@redhat.com>

bharathappali force-pushed the add-manifests branch from 99df86a to 23313c7 Compare August 10, 2023 11:55

dinogun reviewed Aug 11, 2023

View reviewed changes

bharathappali added 4 commits August 11, 2023 12:29

Removed old docker file and updated image name in yamls

592033b

Signed-off-by: bharathappali <abharath@redhat.com>

tag to 0.0.1 and remove unwanted yamls

3b38c78

Signed-off-by: bharathappali <abharath@redhat.com>

Update dynamic yaml

bd74e52

Signed-off-by: bharathappali <abharath@redhat.com>

Update tag in deploy.sh

b6eed77

Signed-off-by: bharathappali <abharath@redhat.com>

dinogun approved these changes Aug 11, 2023

View reviewed changes

dinogun merged commit ede2860 into kruize:mvp_demo Aug 11, 2023

		export const KRUIZE_API_URL = process.env.KRUIZE_APP_API_URL \|\| undefined;
		export const KRUIZE_API_PORT = process.env.KRUIZE_APP_API_PORT \|\| undefined;

		@@ -1,4 +1,17 @@
		#!/bin/bash

		export KRUIZE_UI_IMAGE="docker.io/bharathappali/kruize-ui:central-config"

Add manifests #32

Add manifests #32

Conversation

bharathappali commented Jul 14, 2023

bharathappali commented Jul 24, 2023

CodiumAI-Agent commented Jul 24, 2023

PR Analysis

PR Feedback

How to use

bharathappali commented Jul 24, 2023

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

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

bhanvimenghani Aug 9, 2023 • 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

dinogun left a comment

Choose a reason for hiding this comment

bhanvimenghani Aug 9, 2023 •

edited

Loading