Skip to content

Add Edit Container... command#769

Merged
MicroFish91 merged 18 commits intomainfrom
mwf/presidential-pink
Nov 27, 2024
Merged

Add Edit Container... command#769
MicroFish91 merged 18 commits intomainfrom
mwf/presidential-pink

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Oct 11, 2024

Replace the existing Update Container Image... on the container app item with an Update Container... command now that the containers view has been added.

Before:
image

After:
image

Also adds support for the new activity log view. Examples:

(Updating using existing image)
image

(Updating using workspace project and building image from scratch)
image

Update: Changed to use the Edit Container... instead of Update Container...

@MicroFish91 MicroFish91 requested a review from a team as a code owner October 11, 2024 20:50
@MicroFish91 MicroFish91 changed the title Add and wire-up Update Container... command Add Update Container... command Oct 11, 2024
@MicroFish91 MicroFish91 marked this pull request as draft October 11, 2024 21:19
@MicroFish91 MicroFish91 marked this pull request as ready for review October 22, 2024 21:29
Base automatically changed from mwf/fine-emerald to main November 25, 2024 18:55
Copy link
Copy Markdown
Member

@nturinski nturinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After seeing this quick pick in the UI, I feel like it may be kind of confusing.

{A34837F9-F9CD-44FB-A954-7A093A93F49C}

Also, it's a bit odd to me that we have a yes, no, and no, don't ask again. There is probably a scenario where someone would want yes, and don't ask again as well.

I'll try messing with the quick pick a bit to see if we can add a checkbox or button that could be the "don't ask again". Not blocking it over that, just something that I wanted to bring up.

Comment thread src/commands/updateContainer/updateContainer.ts Outdated
Comment thread src/utils/pickItem/pickContainer.ts Outdated

let containersIdx: number;
if (ContainersItem.isContainersItem(item)) {
// The 'updateContainer' command should only show up on a 'ContainersItem' when it only has one container, else the command would show up on the 'ContainerItem'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit, but if it's an easy change you might want to consider it.

I think that it might be better to always show the command on the ContainersItem. Can we do that and when there are multiple containers just ask the user to pick one? You already have the pick container logic above, you might be able to reuse that.

I believe that users don't like when commands are missing, we want to be consistent so they can do things in the same way each time

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point, that's also how we have it implemented for picking revisions so this would help keep that pattern more consistent. I will make a change in a follow-up PR to add this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the command is named "update container" while the context is the reverse, "container update context". If there's not a good reason for this, then I'd prefer if they matched.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm bouncing this one to @nturinski 🤭

I think he asked in a previous meeting people's preferences and was trying to improve naming conventions... but I can't remember who all was there to chime in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah. I believe the conversation was basically anything that is a class/context should be noun (in this case, Container) prefixed so that in an alphabetical list, it's easy to see all of the Container related files grouped together. However, since commands have been following the "verbNoun" format, I was fine with the reverse order specifically for commands.

Comment thread package.json
Comment on lines +201 to +205
{
"command": "containerApps.updateContainer",
"title": "%containerApps.updateContainer%",
"category": "Azure Container Apps"
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't want to block this PR from merging, but I think we need to discuss the possibility of rewording the command to make it more clear to users what's happening.

We need users to understand that there are pretty much two main actions for container apps:

  1. Editing their Container App configuration (scale rules, etc.) - currently this is "Edit Container App..."
  2. Updating the image running in their containers (within the Container App) - currently this is "Update Container..."

Nathan and I think that "Update Container..." might be too close to "Edit Container App...". What about using Deploy instead of Update?

Whatever we decide to do, we can make that change after this is merged.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'll try to squeeze this discussion into one of our next group meetings (probably just the engineers for now). I think these sorts of improvements overlap a ton with my big goal for the next release, and I have some extra context to provide that would be helpful.

@MicroFish91
Copy link
Copy Markdown
Contributor Author

MicroFish91 commented Nov 26, 2024

Also, it's a bit odd to me that we have a yes, no, and no, don't ask again. There is probably a scenario where someone would want yes, and don't ask again as well.

Although a user could want yes and don't ask again, I would think we should naturally prevent this scenario as I think it's a bad pattern to enable since it kind of goes against the point of batching revision edits.

I do kind of agree that it looks a little funny though, but I'm not sure what the better alternative is yet, may be worth discussing in our next meeting with Anthony

@MicroFish91 MicroFish91 changed the title Add Update Container... command Add Edit Container... command Nov 27, 2024
@microsoft microsoft locked and limited conversation to collaborators Jan 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants