feat: add configmaps & secrets to kubernetes #7681

cdrage · 2024-06-17T20:29:42Z

feat: add configmaps & secrets to kubernetes

What does this PR do?

Adds ConfigMaps and Secrets to Kubernetes
Ability to delete / view / edit both configmap and secrets

Screenshot / video of UI

Screen.Recording.2024-06-17.at.4.23.56.PM.mov

What issues does this PR fix or reference?

Closes #7342
Closes #7190 (its the last part of it)

How to test this PR?

Tests are covering the bug fix or the new feature

Create a ConfigMap and Secrets on your k8s cluster.
Delete / Edit / View them

Signed-off-by: Charlie Drage [email protected]

cdrage · 2024-06-17T20:31:38Z

FYI the icon is the "rocket" temporary until #7680 has a PR.

Apologizes for the large PR! Tried splitting it up a bit, but a lot of the tests are intertwined.

Most of this code is very similar to #7640 however.

deboer-tim

Code worked fine for me in testing and added a few comments, but this PR is so massive that I haven't gone through it all. Parts of this really should be broken out so that we can review and merge independently.

deboer-tim · 2024-06-18T18:25:30Z

packages/renderer/src/AppNavigation.svelte

@@ -34,11 +35,13 @@ import { contributions } from './stores/contribs';
 import { imagesInfos } from './stores/images';
 import { kubernetesContexts } from './stores/kubernetes-contexts';
 import {
+  kubernetesCurrentContextConfigMapsFiltered,


Should be the unfiltered, not the filtered store here.

deboer-tim · 2024-06-18T18:25:58Z

packages/renderer/src/AppNavigation.svelte

  kubernetesCurrentContextDeployments,
  kubernetesCurrentContextIngresses,
  kubernetesCurrentContextNodes,
  kubernetesCurrentContextPersistentVolumeClaims,
  kubernetesCurrentContextRoutes,
+  kubernetesCurrentContextSecretsFiltered,


Should be the unfiltered, not the filtered store here.

deboer-tim · 2024-06-18T18:26:56Z

packages/renderer/src/lib/configmaps-secrets/ConfigMapSecretUI.ts

@@ -0,0 +1,27 @@
+/**********************************************************************
+ * Copyright (C) 2023 Red Hat, Inc.


deboer-tim · 2024-06-18T18:32:35Z

packages/renderer/src/lib/configmaps-secrets/ConfigMapSecretList.svelte

+          if (isSecret) {
+            await window.kubernetesDeleteSecret(configmapSecret.name);
+          } else {
+            await window.kubernetesDeleteConfigMap((configmapSecret as ConfigMapSecretUI).name);


I think it would be worth the readability/type checking if you created a configmapSecretUtils.isConfigMap() and just turned this into a configmapSecretUtils.isSecret() {...} else if configmapSecretUtils.isConfigMap() {...}

I think you can remove the as ConfigMapSecretUI now?

deboer-tim · 2024-06-18T18:38:41Z

packages/renderer/src/lib/ui/Label.svelte

@@ -3,12 +3,13 @@ import { Tooltip } from '@podman-desktop/ui-svelte';

 export let name = '';
 export let tip = '';
+export let capitalize = true;


I went with 'capitalize on' because it was on the first instance I made a component out of, but most of the files that have used it since haven't needed this. As a component I wonder if it's better if capitalize is off by default, i.e. an optional feature instead.

This was delivered via #7788, can remove it here.

deboer-tim · 2024-06-18T18:48:52Z

packages/main/src/plugin/color-registry.ts

@@ -1157,6 +1157,17 @@ export class ColorRegistry {
      dark: colorPalette.gray[100],
      light: colorPalette.gray[400],
    });
+    // If it's considered "good" (like a successful operation), use green
+    this.registerColor(`${status}good`, {


To me there's a disconnect between the name 'good' and the actual use 'config map exists'. I think we should either just use an existing color (e.g. 'updated' from #7526) or name it something more generic like (object) 'exists'.

deboer-tim · 2024-06-18T18:52:40Z

packages/main/src/plugin/color-registry.ts

+    });
+    // If it's "secure" (like a successful TLS connection), use yellow
+    // as often keys are yellow in the real world / "caution" / padlock colour uses on other GUIs
+    this.registerColor(`${status}secure`, {


Amber is more of a warning color than a yellow lock. I think @ekidneyrh should weigh in on what color 'secure' is, whether we need another color in the palette or should just use 'status-connected' (green 600).

If we do go the route of a 'secure' color different from connected, I'd wonder if Log In or other places should use that instead of -connected.

cdrage · 2024-06-18T19:55:04Z

@deboer-tim I've updated the PR with a new commit with the suggested changes:

Added isConfigMap / made the if statements more intuitive
Use green for the "running" / working configmap and secret.
Updated the incorrect filtered areas

Still unsure what to do with Label? I guess another PR / issue, since removing the capitalization will affect other parts of PD.

deboer-tim · 2024-06-19T13:08:05Z

packages/renderer/src/lib/configmaps-secrets/configmap-secret-utils.ts

+
+  // If it is a configMap, then it will **not** have a type property
+  isConfigMap(storage: V1ConfigMap | V1Secret): storage is V1ConfigMap {
+    return !('type' in storage);


It would probably be better to have a 'positive' check for something unique about config map vs 'it's not a secret' 😆. Is there nothing that's only in configmap?

actually yes! I'll update it to also check for 'binaryData' too.

deboer-tim · 2024-06-19T13:09:05Z

packages/renderer/src/lib/configmaps-secrets/ConfigMapSecretList.svelte

+          if (isSecret) {
+            await window.kubernetesDeleteSecret(configmapSecret.name);
+          } else {
+            await window.kubernetesDeleteConfigMap((configmapSecret as ConfigMapSecretUI).name);


I think you can remove the as ConfigMapSecretUI now?

deboer-tim · 2024-06-19T13:12:19Z

packages/renderer/src/lib/configmaps-secrets/configmap-secret-utils.ts

@@ -0,0 +1,56 @@
+/**********************************************************************
+ * Copyright (C) 2023 Red Hat, Inc.


Should be 2024

feloy · 2024-06-19T13:51:57Z

The values in the Type column for secrets are the ones displayed in kubectl get secret, but I would prefer to have the ones used when using kubectl create secret <type> (docker-registry, generic or tls). WDYT?

For specific secrets (docker, tls), I wonder if we need to display the keys, as they are standard (and not really useful to the user).

kubectl get cm / secret display the number of data, without the names of the keys. I wonder if it is relevant to display the key names in the list, as it can easily be unreadable

feloy · 2024-06-19T13:55:14Z

I would prefer to have the display of the data moved up one level:

Details

Type      Opaque
Immutable  No


Data

key1       value1
key2       value2
key3       value3
key4       value4
key5       value5

cdrage · 2024-06-19T18:11:16Z

Thank you for the suggestions @feloy I agree. Having keys on there is not ideal. Other GUI's do it, but because of our spacin and simplicity it's nice to say the length / how many rather than every name of the key..

I've updated the UI with the "Summary" changes as well as the list changes. I agree it looks a lot better.

packages/renderer/src/lib/kube/details/KubeSecretArtifact.svelte

cdrage · 2024-06-19T20:36:37Z

Updated so now they are on newlines @feloy

### What does this PR do? * Adds ConfigMaps and Secrets to Kubernetes * Ability to delete / view / edit both configmap and secrets ### Screenshot / video of UI  ### What issues does this PR fix or reference?  Closes containers#7342 Closes containers#7190 ### How to test this PR?  - [X] Tests are covering the bug fix or the new feature 1. Create a ConfigMap and Secrets on your k8s cluster. 2. Delete / Edit / View them Signed-off-by: Charlie Drage <[email protected]>

Signed-off-by: Charlie Drage <[email protected]>

When creating Label I left 'capitalize' the default because the first Label (ProviderInfo) needed it, but none of the other instances do. PR containers#7681 it requires no capitalization so this just separates that out - making capitalization off by default and only used by the one use that requires it, ProviderInfo. Prereq of containers#7681. Signed-off-by: Tim deBoer <[email protected]>

When creating Label I left 'capitalize' the default because the first Label (ProviderInfo) needed it, but none of the other instances do. PR #7681 it requires no capitalization so this just separates that out - making capitalization off by default and only used by the one use that requires it, ProviderInfo. Prereq of #7681. Signed-off-by: Tim deBoer <[email protected]>

cdrage requested review from benoitf and a team as code owners June 17, 2024 20:29

cdrage requested review from jeffmaury and feloy and removed request for a team June 17, 2024 20:29

cdrage force-pushed the add-configmaps-secrets branch from 6770bb2 to 17bde4d Compare June 18, 2024 13:57

deboer-tim reviewed Jun 18, 2024

View reviewed changes

cdrage force-pushed the add-configmaps-secrets branch from f8642c0 to 37539b2 Compare June 18, 2024 19:34

cdrage force-pushed the add-configmaps-secrets branch from 37539b2 to a6dd500 Compare June 18, 2024 19:57

deboer-tim reviewed Jun 19, 2024

View reviewed changes

cdrage force-pushed the add-configmaps-secrets branch from a6dd500 to a3f258b Compare June 19, 2024 13:14

cdrage force-pushed the add-configmaps-secrets branch from 857ff24 to 4e5f929 Compare June 19, 2024 18:52

feloy reviewed Jun 19, 2024

View reviewed changes

packages/renderer/src/lib/kube/details/KubeSecretArtifact.svelte Outdated Show resolved Hide resolved

cdrage force-pushed the add-configmaps-secrets branch 3 times, most recently from 7d5e1e9 to 04d2da1 Compare June 19, 2024 23:04

cdrage added 6 commits June 19, 2024 19:05

update based on review

7f98b10

Signed-off-by: Charlie Drage <[email protected]>

update based on review

2b4f3c4

Signed-off-by: Charlie Drage <[email protected]>

update list page, artifact summaries

e8f33d8

keys on each newline

ab7cace

Signed-off-by: Charlie Drage <[email protected]>

update tests

7dd5483

Signed-off-by: Charlie Drage <[email protected]>

cdrage force-pushed the add-configmaps-secrets branch from 04d2da1 to 7dd5483 Compare June 19, 2024 23:05

deboer-tim mentioned this pull request Jun 23, 2024

chore: optional label capitalization #7788

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add configmaps & secrets to kubernetes #7681

feat: add configmaps & secrets to kubernetes #7681

cdrage commented Jun 17, 2024 •

edited

Loading

cdrage commented Jun 17, 2024

deboer-tim left a comment

deboer-tim Jun 18, 2024

deboer-tim Jun 18, 2024

deboer-tim Jun 18, 2024

deboer-tim Jun 18, 2024

deboer-tim Jun 19, 2024

deboer-tim Jun 18, 2024

deboer-tim Jun 24, 2024

deboer-tim Jun 18, 2024

deboer-tim Jun 18, 2024

cdrage commented Jun 18, 2024

deboer-tim Jun 19, 2024

cdrage Jun 19, 2024

deboer-tim Jun 19, 2024

deboer-tim Jun 19, 2024

feloy commented Jun 19, 2024

feloy commented Jun 19, 2024

cdrage commented Jun 19, 2024

cdrage commented Jun 19, 2024

		@@ -0,0 +1,27 @@
		/**********************************************************************
		* Copyright (C) 2023 Red Hat, Inc.

		@@ -0,0 +1,56 @@
		/**********************************************************************
		* Copyright (C) 2023 Red Hat, Inc.

feat: add configmaps & secrets to kubernetes #7681

Are you sure you want to change the base?

feat: add configmaps & secrets to kubernetes #7681

Conversation

cdrage commented Jun 17, 2024 • edited Loading

What does this PR do?

Screenshot / video of UI

What issues does this PR fix or reference?

How to test this PR?

cdrage commented Jun 17, 2024

deboer-tim 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

cdrage commented Jun 18, 2024

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

feloy commented Jun 19, 2024

feloy commented Jun 19, 2024

cdrage commented Jun 19, 2024

cdrage commented Jun 19, 2024

cdrage commented Jun 17, 2024 •

edited

Loading