Add Secrets view and deploy with Secrets to extension #2288

dotNomad · 2024-09-18T18:03:27Z

This PR adds a new Secrets section to the Home view.

Some new features:

Secret values present are sent up on Deploy and added as environment variables on Connect. If the env var is present it's value is overwritten
When changing Deployments the provided Secret values are cleared
When a successful Deployment occurs the provided Secret values are cleared
When a Configuration is changed, but the Deployment stays the same, the shared Secrets keep their values in the UI
The number of Secrets with values is displayed as a Count badge on the Secrets section header to help the user know what is going to be sent up
When editing a Secret the input will be selected so all of the value is highlighted for easy overwrite (similar to the rename functionality in VS Code)

Preview

CleanShot.2024-09-19.at.16.43.00.mp4

Intent

Resolves #2252, #2257, #2258

Type of Change

- Bug Fix
- New Feature
- Breaking Change
- Documentation
- Refactor
- Tooling

Approach

The approach here was to expand previously made components, and create new ones (when necessary) to allow for the new model of "editing" a Tree Item description.

In this case editing the value of the Secret items.

Actions are used to edit and clear Secrets. Instead of clicking directly the action button is more clear, and avoids issues with duplicated click events when the input field is already up.

When editing a Secret value the action buttons are removed from the UI until editing is complete. This was done to avoid confusing the user and dealing with tricky cases like what clicking "edit" or "clear" should do while the input is open. It also provides more space for the input field which is horizontally constrained.

Directions for Reviewers

Ensure that Tree Item descriptions function and are styled as they previously were.

Ensure the new Tree Item with description input (the Secrets) function as expected.

Ensure that Secrets with values are sent up and are correct on the Connect Server. Secrets without values should remain the same across multiple re-deployments.

sagerb

Some early feedback, happy to discuss or explain my observations. Thanks!

extensions/vscode/src/views/homeView.ts

sagerb · 2024-09-18T23:00:52Z

extensions/vscode/webviews/homeView/src/components/views/secrets/Secret.vue

+import { ActionButton } from "src/components/ActionToolbar.vue";
+
+interface Props {
+  name: string;


I would have expected that this component would be a UI component that would accept its name and value through a prop and emit a change when the value changed. I would then have expected the view using this component to have to deal with the storage of the secret value.

While this approach will work, it makes this component extremely dedicated to this particular input vs. having a component that allows its value to be edited. I think we would benefit from having it be a bit more general purpose, especially when you think of using it for environment variables as well.

I actually started writing it this way at the start, but found it more difficult to keep the home.secrets Map in line with the value that was being v-modeled in Secret.vue instead I made it just reflect the state of home.secrets.get('name') for the name key provided

The main difficulty when using v-model was grabbing the most recent value when swapping over to the input mode. It worked great when setting the value, but not when the value changed out from under the component.

Did you get a chance to play with the approach we discussed via zoom? That would be where outside component would pass in the map value from the store and then update the store when receiving the change emitted event from the component?

I did mess around with this, and it ended up re-creating the Map in the Secrets component a bit as an array so each v-model accessed an index in that array. It felt duplicative to me.

extensions/vscode/webviews/homeView/src/stores/home.ts

sagerb · 2024-09-18T23:12:21Z

extensions/vscode/webviews/homeView/src/stores/home.ts

@@ -57,6 +60,32 @@ export const useHomeStore = defineStore("home", () => {
    },
  );

+  watch(
+    [selectedContentRecord, selectedConfiguration],


I was wondering about simplifying this into two different watches.

on change of content record, remove all secrets.

on change of config, set the secret from the values present and clear the ones that have gone away.

I've spent a few minutes making sure I understand what your implementation was doing and I think splitting it may avoid that for others in the future.

I initially planned to do this as two separate watches due to the reason you stated, but I was worried about any timing issues that could have spawned from that. For example, some code somewhere could potentially change the content record first then the configuration, and code elsewhere does the opposite potentially leaving secrets in a different state.

Keeping it as I have it avoids that by ensuring secrets is in the correct state no matter if one or both change.

Let me know if I'm making a bad assumption though. I would like to simplify if possible.

Let me know if I'm making a bad assumption though. I would like to simplify if possible.

Would the opposite order really end up not equaling the same outcome?

Content-Record changes and then Configuration changes

Content-Record change removes all secrets

Configuration change has no secrets set, so it does nothing

Configuration changes and then Content-Record changes

Configuration change sets the secret from the values present and clears the ones that have gone away

Content-Record change removes all secrets

In both cases, no secrets are set after both of these events.

Am I seeing this wrong?

No I think you got it, let me play around with this split into two watches and see if I can break anything. Your logic is sound.

I played around with this a bit, and it ended up being really close to a duplication for both watchers. If you have a code block suggestion we can take a look.

I'm probably just missing something in my quick analysis, so I'll defer to yours. Let's go with what you've got working. Thanks for indulging me!

If the element is an input it should not have lower opacity Flex is necessary for the description to take up the remaining space, and behave similarly to the previous iteration of TreeItem

Do not show actions when editing the value

dotNomad · 2024-09-19T23:37:18Z

extensions/vscode/webviews/homeView/src/components/tree/TreeItem.vue

@@ -151,6 +155,7 @@ const toggleExpanded = () => {

    .tree-item-label-container {
      flex: 1;
+      display: flex;


Had to bring in flex for the label container so that the input in the description span could be displayed the way we want. This required bringing down the

min-width: 0; overflow: hidden; text-overflow: ellipsis;

CSS for dealing with overflowing with ellipsis.

dotNomad · 2024-09-19T23:37:48Z

extensions/vscode/webviews/homeView/src/components/tree/TreeItem.vue

      }

      .tree-item-description {
+        line-height: 22px;


line-height has returned. Now that we are using display: flex here the span is no longer being rendered the same way. Without this the height changes, and the description wasn't on the baseline.

dotNomad · 2024-09-19T23:38:36Z

extensions/vscode/webviews/homeView/src/components/tree/TreeItem.vue

        font-size: 0.9em;
        margin-left: 0.5em;
-        opacity: 0.7;


Since description can contain an input now we don't want the description to always be opacity: 0.7. Now we apply this only if the description doesn't contain an input (see below)

dotNomad · 2024-09-19T23:39:04Z

extensions/vscode/webviews/homeView/src/components/tree/TreeItem.vue

        font-size: 0.9em;
        margin-left: 0.5em;
-        opacity: 0.7;
+        flex-shrink: 100000;


This retains the behavior we had before where the description would shrink before the title.

extensions/vscode/webviews/homeView/src/components/views/secrets/Secret.vue

dotNomad · 2024-09-19T23:40:35Z

extensions/vscode/webviews/homeView/src/components/views/secrets/Secrets.vue

+  {
+    label: "Clear Values for all Secrets",
+    codicon: "codicon-clear-all",
+    fn: () => {
+      home.secrets.forEach((_, key) => {
+        home.secrets.set(key, undefined);
+      });
+    },
+  },


I could optionally show this only if 1 or more secrets have values?

The more I use this the more I want to do this, since the button moves if the count badge is showing or not.

dotNomad · 2024-09-19T23:41:32Z

extensions/vscode/webviews/homeView/src/style.css

+::selection {
+  background-color: rgba(195, 183, 184, 0.15);
+}


This preserves the input selection color, and matches the highlight color across VS Code. This doesn't appear to be a variable, instead it is hardcoded which is interesting. I did check, it is consistent across themes.

dotNomad · 2024-09-19T23:42:03Z

extensions/vscode/webviews/homeView/src/stores/home.ts

@@ -57,6 +60,32 @@ export const useHomeStore = defineStore("home", () => {
    },
  );

+  watch(
+    [selectedContentRecord, selectedConfiguration],


I played around with this a bit, and it ended up being really close to a duplication for both watchers. If you have a code block suggestion we can take a look.

sagerb

Added a few responses, which I'll leave to your discretion. I'm approving it so you can determine where you do any followup.

dotNomad force-pushed the dotnomad/deploy-with-secrets branch from 6feea3a to 0db4846 Compare September 18, 2024 20:19

Base automatically changed from dotnomad/deploy-with-secrets to main September 18, 2024 21:30

dotNomad force-pushed the dotnomad/secrets-view branch from 1a31ac8 to 40f4200 Compare September 18, 2024 21:48

sagerb reviewed Sep 18, 2024

View reviewed changes

dotNomad added 16 commits September 19, 2024 16:14

Add secret name to value map to home store

006dbf6

Optionally send secrets in deploy msg

78e8cc7

Add clearSecretValues action to store

c5de584

Clear secrets on successful publish

393bfc9

Create secrets view

e7b4279

Select all text in secret input

2ef1975

Use clear-all for clear secrets codicon

06f3220

Handle Secret input state when cleared

8895901

Add selection highlight color to CSS

fff20db

Start styling sidebar input

1ecc282

Narrow import of isConfigurationError for tests

399b651

Correctly set secrets empty with no valid config

48c1d6a

Split SidebarInput into its own component

13a711a

Allow TreeItem to take any element for description

26c8c83

If the element is an input it should not have lower opacity Flex is necessary for the description to take up the remaining space, and behave similarly to the previous iteration of TreeItem

Use SidebarInput for Secrets item

55d5b0d

Do not show actions when editing the value

Show count of secrets with value in tree view

723dd4f

dotNomad force-pushed the dotnomad/secrets-view branch from f2078cf to 723dd4f Compare September 19, 2024 23:20

dotNomad commented Sep 19, 2024

View reviewed changes

dotNomad changed the title ~~Dotnomad/secrets view~~ Add Secrets view and deploy with Secrets to extension Sep 19, 2024

dotNomad marked this pull request as ready for review September 19, 2024 23:44

dotNomad requested review from kgartland-rstudio and marcosnav as code owners September 19, 2024 23:44

sagerb approved these changes Sep 20, 2024

View reviewed changes

dotNomad added 3 commits September 20, 2024 12:01

Use codicon-remove for reseting secret value

45e5056

De-emphasize secrets without values

9dd300b

Keep emphasis when editing a Secret

144b185

dotNomad merged commit 5e563c7 into main Sep 20, 2024
13 checks passed

dotNomad deleted the dotnomad/secrets-view branch September 20, 2024 23:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Secrets view and deploy with Secrets to extension #2288

Add Secrets view and deploy with Secrets to extension #2288

dotNomad commented Sep 18, 2024 •

edited

Loading

sagerb left a comment

sagerb Sep 18, 2024

dotNomad Sep 18, 2024

dotNomad Sep 18, 2024

sagerb Sep 20, 2024

dotNomad Sep 20, 2024

sagerb Sep 18, 2024

dotNomad Sep 18, 2024

sagerb Sep 19, 2024 •

edited

Loading

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

sagerb Sep 20, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

dotNomad Sep 19, 2024

sagerb left a comment

Add Secrets view and deploy with Secrets to extension #2288

Add Secrets view and deploy with Secrets to extension #2288

Conversation

dotNomad commented Sep 18, 2024 • edited Loading

Intent

Type of Change

Approach

Directions for Reviewers

sagerb 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

sagerb Sep 19, 2024 • 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

sagerb left a comment

Choose a reason for hiding this comment

dotNomad commented Sep 18, 2024 •

edited

Loading

sagerb Sep 19, 2024 •

edited

Loading