Skip to content

Add support for choosing a source directory when deploying from a workspace project#586

Merged
MicroFish91 merged 59 commits intomainfrom
mwf/dwp-src
Feb 6, 2024
Merged

Add support for choosing a source directory when deploying from a workspace project#586
MicroFish91 merged 59 commits intomainfrom
mwf/dwp-src

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Jan 9, 2024

Basically add support to select a source folder (what gets deployed to ACR build) if a Dockerfile is selected that's not in the project root. Previously, we were defaulting the source folder as the parent directory containing the selected Dockerfile.

Closes #544

@MicroFish91 MicroFish91 changed the base branch from main to mwf/deploy-workspace-project-api January 9, 2024 18:42
Base automatically changed from mwf/deploy-workspace-project-api to main January 24, 2024 19:33
@MicroFish91 MicroFish91 marked this pull request as ready for review January 25, 2024 03:27
@MicroFish91 MicroFish91 requested a review from a team as a code owner January 25, 2024 03:27
placeHolder: localize('sourceDirectoryPick', 'Choose your source code directory')
});

context.srcPath = (await context.ui.showOpenDialog({
Copy link
Copy Markdown
Member

@nturinski nturinski Jan 26, 2024

Choose a reason for hiding this comment

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

I'd prefer we show a workspace quickpick with the folders (including .) in the workspace first, and then allow the user to browse to the folder if they don't have it opened.

The reason being that it's always jarring to have the open folder dialog come up in the middle of a wizard and it's kind of hard to give context to the user as to what they need to open since there's no placeholder or anything.

I see that's the point of having the first quickpick that only contains browse, but I think that's still a less than ideal experience.

EDIT: Or actually maybe we default them to the folder where their dockerfile is located, and then let them browse if they'd want to change it.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Jan 26, 2024

Choose a reason for hiding this comment

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

That's fair, my only question is, if we end up suggesting the wrong location using the Dockerfile as reference, do you think most people will think it intuitive enough to hit browse to pick the correct location? Or do you think people would just automatically pick what we suggested without double checking?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh I see you also mentioned showing "." first, let's maybe talking offline so I can fully understand which places could make sense to show

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.

I think users would understand it.

I think it's similar to Static Web Apps, but basically, it shows your root folder and every sub-directory as a quickpick with browse at the bottom.

I don't think it would be anymore confusing that just asking where the source folder is, at least. It's also an additional click which may be annoying (especially if the src folder is one that we could list for users from the get-go).

@MicroFish91
Copy link
Copy Markdown
Contributor Author

MicroFish91 commented Jan 29, 2024

Now displays all the directories from root to Dockerfile as selectable options. Still provides a browse button at the end just in case.

Example:
image

Example:
image

picks.push({ label: '.' + p, data: rootPath + p });
}

(picks.at(-1) as IAzureQuickPickItem).description = 'dockerfile';
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.

Would it be more helpful to show the name of the dockerfile instead? I'm not sure exactly what the intention of the description is.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Feb 1, 2024

Choose a reason for hiding this comment

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

By this step, the user has already chosen a Dockerfile. This description is just indicating to the user that the chosen Dockerfile is present at this folder level.

Yeah, we could show the full name of the Dockerfile instead

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.

Ohh, I see. 🤔

Shoot, I don't feel like that is super obvious based on just that description. I don't really have any great alternative suggestions.

Part of me is wondering how useful that information is to the user? Because where their dockerfile is may not really be that relevant to where their source code is anyway.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Feb 1, 2024

Choose a reason for hiding this comment

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

Yeah, think it's worth waiting for PM feedback for this one then? I don't really have strong opinions either way tbh. I was also on the fence about whether it looked good / was useful.

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.

I think we'd be safe to just remove it to be honest. My gut feeling is that they won't really care.

I think it's more likely to confuse a user and I don't think knowing that is where the dockerfile is located is super relevant for this step anyway.

@MicroFish91 MicroFish91 merged commit 237dad3 into main Feb 6, 2024
@MicroFish91 MicroFish91 deleted the mwf/dwp-src branch February 6, 2024 23:56
@microsoft microsoft locked and limited conversation to collaborators Mar 23, 2024
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.

Add support for choosing a source directory when deploying from a workspace project

2 participants