feat/chore: Add App Delete Functionality and Refactor Code #36

arkaprovob · 2023-10-06T11:34:29Z

This pull request is for the following changes:

Add App Delete Functionality:
- Introduces the App Delete feature, following the same process as application distribution upload.
- During app deletion, the distribution zip file includes only the splaship.yaml file with the target application name and mandatory details related to the application.
- Sets the exclude flag to true for deleted apps.
**Refactor Code Following App Delete Functionality Addition: **
- Refactors the codebase to improve readability and maintainability.
- No changes in functionality, only code organization and style enhancements.

Introduces the app delete feature, following the same process as application distribution upload. For deleting an app the dist zip file will include only the spaship.yaml file with the target application name and mandatory details. Also, exclude from environment is set to 'true'.

soumyadip007 · 2023-10-09T05:24:01Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+                    EventManager eventManager, @Named("deNamespace") String ns) {
+        this.ocClient = ocClient;
+        this.eventManager = eventManager;
+        domain = ConfigProvider.getConfig().getValue("operator.domain.name", String.class);


@arkaprovob can you use a variable instead of directly using the string?

private static final String OPERATOR_DOMAIN_NAME = "operator.domain.name";

Great observation, Soumya! I've created a GitHub issue for this. Instead of modifying just this file, let's consider refactoring other classes to ensure that these changes are applied consistently throughout, Let's handle it separately.

Sure @arkaprovob

soumyadip007

@arkaprovob I've added a few comments in this PR. Please check

soumyadip007 · 2023-10-09T05:24:15Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+        this.ocClient = ocClient;
+        this.eventManager = eventManager;
+        domain = ConfigProvider.getConfig().getValue("operator.domain.name", String.class);
+        routerDomain = ConfigProvider.getConfig().getValue("operator.router.domain.name", String.class);


private static final String OPERATOR_DOMAIN_NAME = "operator.domain.name";

Great observation, Soumya! I've created a GitHub issue for this. Instead of modifying just this file, let's consider refactoring other classes to ensure that these changes are applied consistently throughout, Let's handle it separately.

Sure @arkaprovob

soumyadip007 · 2023-10-09T05:27:12Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+
+        ReUsableItems.releaseLock(environment.getIdentification());
+        LOG.debug("\n");
+        // todo change the status code type to enum instead of int for better readability


Optional : As you are following // TODO: format in the entire code, you can format // todo with // TODO:

created an issue for the same #38

soumyadip007 · 2023-10-09T05:27:17Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+        if (!envExists)
+            createNewEnvironment(environment);
+        String sideCarSvcUrl = environmentSidecarUrl(environment);
+        // todo mutability is not good, we should not change the state of the object


Optional : As you are following // TODO: format in the entire code, you can format // todo with // TODO:

created an issue for the same #38

src/main/java/io/spaship/operator/service/k8s/Operator.java

soumyadip007 · 2023-10-09T05:31:44Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+    private void createNewTenantNamespace(Environment environment, Map<String, String> templateParameters) {
+        var k8sNSList = ocClient
+                .templates()
+                .load(Operations.class.getResourceAsStream("/openshift/mpp-namespace-template.yaml"))


could you please add this path to a variable ? (also if possible you can provide an option to read this path from the env variable if you think it's feasible)

private final String OPENSHIFT_NAMESPACE_PATH = "/openshift/mpp-namespace-template.yaml";

Good observation I have created a GitHub issue for the same.

src/main/java/io/spaship/operator/service/k8s/Operator.java

soumyadip007 · 2023-10-09T05:48:38Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            }
+            if (item instanceof PersistentVolumeClaim pvc) {
+                LOG.debug("creating new pvc in K8s, tracing = {}", tracing);
+                ocClient.persistentVolumeClaims().inNamespace(nameSpace).createOrReplace(pvc);


Note : There might be a potential issue of exception, can we handle it here ! (Optional)

As long as the parameter of the function KubernetesList result not null, there should be no other issues, thanks to Fabric8 library handling the rest.
I have created #45 to add a null check for the 'KubernetesList result' parameter.

soumyadip007 · 2023-10-09T05:48:45Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            }
+            if (item instanceof Route route) {
+                LOG.debug("creating new Route in K8s, tracing = {}", tracing);
+                ocClient.routes().inNamespace(nameSpace).createOrReplace(route);


Note : There might be a potential issue of exception, can we handle it here ! (Optional)

As long as the parameter of the function KubernetesList result not null, there should be no other issues, thanks to Fabric8 library handling the rest.
I have created this issue to add a null check for the 'KubernetesList result' parameter.

Okay @arkaprovob

soumyadip007 · 2023-10-09T05:48:51Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            }
+            if (item instanceof ConfigMap cm) {
+                LOG.debug("creating new ConfigMap in K8s, tracing = {}", tracing);
+                ocClient.configMaps().inNamespace(nameSpace).createOrReplace(cm);


Note : There might be a potential issue of exception, can we handle it here ! (Optional)

As long as the parameter of the function KubernetesList result not null, there should be no other issues, thanks to Fabric8 library handling the rest.
I have created #45 to add a null check for the 'KubernetesList result' parameter.

Okay @arkaprovob

src/main/java/io/spaship/operator/service/k8s/Operator.java

soumyadip007

Hi @arkaprovob , I've gone through you comments. Thanks for creating the respected issues against the comments. We'll go through it and resolve these as a separate issue.

soumyadip007 · 2023-10-17T05:05:22Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+                    EventManager eventManager, @Named("deNamespace") String ns) {
+        this.ocClient = ocClient;
+        this.eventManager = eventManager;
+        domain = ConfigProvider.getConfig().getValue("operator.domain.name", String.class);


Sure @arkaprovob

soumyadip007 · 2023-10-17T05:05:27Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+        this.ocClient = ocClient;
+        this.eventManager = eventManager;
+        domain = ConfigProvider.getConfig().getValue("operator.domain.name", String.class);
+        routerDomain = ConfigProvider.getConfig().getValue("operator.router.domain.name", String.class);


Sure @arkaprovob

soumyadip007 · 2023-10-17T05:05:33Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+
+        ReUsableItems.releaseLock(environment.getIdentification());
+        LOG.debug("\n");
+        // todo change the status code type to enum instead of int for better readability


soumyadip007 · 2023-10-17T05:05:36Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+        if (!envExists)
+            createNewEnvironment(environment);
+        String sideCarSvcUrl = environmentSidecarUrl(environment);
+        // todo mutability is not good, we should not change the state of the object


soumyadip007 · 2023-10-17T05:06:38Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            String type = (String) rule.get("type");
+            Map<String, Object> to = (Map<String, Object>) rule.get("to");
+            String cidrSelectorName = (String) to.get("cidrSelector");
+            return "0.0.0.0/0".equals(cidrSelectorName) && type.equalsIgnoreCase("Deny");


Okay make sense

soumyadip007 · 2023-10-17T05:07:04Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            }
+            if (item instanceof Route route) {
+                LOG.debug("creating new Route in K8s, tracing = {}", tracing);
+                ocClient.routes().inNamespace(nameSpace).createOrReplace(route);


Okay @arkaprovob

src/main/java/io/spaship/operator/service/k8s/Operator.java

soumyadip007 · 2023-10-17T05:09:05Z

src/main/java/io/spaship/operator/service/k8s/Operator.java

+            }
+            if (item instanceof ConfigMap cm) {
+                LOG.debug("creating new ConfigMap in K8s, tracing = {}", tracing);
+                ocClient.configMaps().inNamespace(nameSpace).createOrReplace(cm);


Okay @arkaprovob

arkaprovob added 2 commits October 3, 2023 01:14

chore: Refactor code following the addition of app delete functionality

2bb7ad5

arkaprovob added enhancement New feature or request chore labels Oct 6, 2023

arkaprovob requested a review from soumyadip007 October 6, 2023 11:34

arkaprovob self-assigned this Oct 6, 2023

soumyadip007 reviewed Oct 9, 2023

View reviewed changes

soumyadip007 added hacktoberfest-accepted Hacktoberfest operator refactor labels Oct 9, 2023

This was referenced Oct 17, 2023

Reduce the changes of error and npe: Check for variable Existence #43

Open

Refactor Code: Convert States to Enum #44

Open

Enhance Code quality: Add Null Check #45

Open

soumyadip007 approved these changes Oct 17, 2023

View reviewed changes

soumyadip007 merged commit 172cf9c into spaship:main Oct 17, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/chore: Add App Delete Functionality and Refactor Code #36

feat/chore: Add App Delete Functionality and Refactor Code #36

arkaprovob commented Oct 6, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 left a comment

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 9, 2023

arkaprovob Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 left a comment

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

soumyadip007 Oct 17, 2023

feat/chore: Add App Delete Functionality and Refactor Code #36

feat/chore: Add App Delete Functionality and Refactor Code #36

Conversation

arkaprovob commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soumyadip007 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

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

soumyadip007 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment