-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add editEnvironmentVariableName
command
#796
Conversation
{ | ||
"command": "containerApps.editEnvironmentVariableName", | ||
"when": "view =~ /(azureResourceGroups|azureFocusView)/ && viewItem =~ /environmentVariableItem/i", | ||
"group": "1@1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think technically this should be 1@2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 1@1 is correct, but it's confusing because the one above it is environmentVariablesItem
with an s and they look super similar 😆
import { type EnvironmentVariablesBaseContext } from "../EnvironmentVariablesContext"; | ||
import { type EnvironmentVariableType } from "../addEnvironmentVariable/EnvironmentVariableTypeListStep"; | ||
|
||
export interface EnvironmentVariableEditBaseContext extends EnvironmentVariablesBaseContext, Pick<ISecretContext, 'secretName'> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be very similar to the EnvironmentVariableAddBaseContext
. Any reason to make a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, they are super similar. To simplify a bit, I extended from the add context and just made the environmentVariable
required because it's needed up front for the edits. I think I still prefer to keep a separate context just because I'm trying to keep the two command paths decoupled for the most part (mostly because I don't want to complicate the draft step with both add and edit logic).
import { RevisionDraftUpdateBaseStep } from "../../revisionDraft/RevisionDraftUpdateBaseStep"; | ||
import { type EnvironmentVariableEditContext } from "./EnvironmentVariableEditContext"; | ||
|
||
export class EnvironmentVariableEditDraftStep<T extends EnvironmentVariableEditContext> extends RevisionDraftUpdateBaseStep<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a huge deal, but I feel like this step could almost entirely be consolidated with the "AddStep".
But if you like having them separate just for organization/logical purposes, makes sense to me too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although there are a lot of overlaps, there's enough differences that I'd prefer to leave them uncoupled to reduce complexity if we ever need to come back and alter the behavior.
No description provided.