Add `Dockerfile` port detection to ingress configuration logic by MicroFish91 · Pull Request #449 · microsoft/vscode-azurecontainerapps

MicroFish91 · 2023-09-11T22:41:34Z

Related to: #425

Added a pre-configuration path for ingress when a Dockerfile is detected + tests

…inerapps into mwf/deployWorkspaceProject-I

MicroFish91 · 2023-09-11T22:54:00Z

+        context.targetPort = getDefaultPort(context);
+    }
+
+    // If a container app already exists, activity children will be added automatically in later execute steps


Will add this back in once we add all the activity children support in upcoming PR

bwateratmsft · 2023-09-12T13:06:23Z

+
+    const portRanges: PortRange[] = [];
+    for (const line of lines) {
+        if (/^EXPOSE/i.test(line.trim())) {


Dockerfiles aren't too tough to parse, but it may be easier to use an existing parser. The Dockerfile language service used by the Docker extension has one. https://www.npmjs.com/package/dockerfile-ast

Hmm, I did look into this one a little bit before and I ended up feeling unsure if it removed enough complexity to warrant adding a third party dependency since the scope of use is relatively small here.

From what I can tell, I would still have to loop through each line in search of an expose keyword (fairly similar to what I'm doing already). Also the parsed args list it provides is still made up of raw strings that I have to loop through again to identify ranges - and slashes / so that I can convert them into useable port numbers/ranges.

I think the main benefit I see is that it might reduce the complexity of some of the RegExp that I'm using, but the overall process doesn't seem to get that much simpler. With this context, do you think it would still be beneficial to use the new dep?

That's fair, I think it's fine to manually parse, just make sure you know all the variations that an EXPOSE statement can have.

…crosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-III

nturinski · 2023-09-13T17:18:21Z

+    }
+}
+
+export async function getDockerfileExposePorts(dockerfilePath: string): Promise<PortRange[] | undefined> {


Should call this tryGetDockerfileExposePorts since it can return undefined.

Also feels like that this should just be in its own file.

nturinski · 2023-09-13T17:24:36Z

@@ -0,0 +1,7 @@
+## Modified example from Azure-Samples/acr-build-helloworld-node
+FROM node:15-alpine


I assume that it probably doesn't matter for our unit tests, but should we try to target things other than node:15-alpine?

That's fair, I literally just copy pasted everything from the original azure-sample and just changed the expose sections

nturinski · 2023-09-13T17:33:54Z

+
+import type { PortRange } from "../../extension.bundle";
+
+export interface MockIngressContext {


Just want to confirm, but the reason you're not just using the IngressContext because of the containerApp has too many required properties that aren't worth mocking?

Yeah, that's a big part of the reason

nturinski · 2023-09-13T17:49:31Z

+        for (const [i, ds] of dockerfileSamples.entries()) {
+            const context: MockIngressContext = {
+                dockerfilePath: i === 1 ? undefined : ds.fsPath,
+                alwaysPromptIngress: i === 3


I assume that you're only setting this true for the 4th dockerfile? Maybe add a comment in the expected results which one has this true?

I'm not sure how to do it better, but I think it would be kind of difficult to add tests to this suite if we ever do. Definitely not worth blocking, but I'll keep thinking about it as well.

I have it in the expected results array at the top!

I'll add the =true modifier

nturinski · 2023-09-13T18:07:13Z

+
+    context.dockerfileExposePorts = await getDockerfileExposePorts(context.dockerfilePath);
+
+    if (context.alwaysPromptIngress) {


Small nit, but should probably be at the top of the file so we don't waste any time doing the async call above.

This position is intentional, because it's still nice to grab the expose ports so we can provide port suggestions later down the line (results can be leveraged when we call getDefaultPort again later)

MicroFish91 added 19 commits September 1, 2023 12:15

Partial

a27a2bc

Merge branch 'main' of https://github.com/microsoft/vscode-azureconta…

42d6c19

…inerapps into mwf/deployWorkspaceProject-I

Update validation logic

c13c61e

Wip

0e25f45

Wip tests

1134743

Add tests

426b5e4

Revert main.js

64dccfc

Update test file names

fbffd33

Import type

0fa2f4c

Update test case

91eff67

Make pr smaller

06a7ba9

Fix

d21cdb2

Impl

9e8f37f

Merge with part I

d8d2345

Add back name validation refactors

a9ab3ac

Update var names

dae4d98

Revert old change

830ecb5

nit

3e8ea38

Impl

57e86fe

MicroFish91 force-pushed the mwf/deployWorkspaceProject-III branch from 1da4d49 to 57e86fe Compare September 11, 2023 22:42

MicroFish91 added 2 commits September 11, 2023 15:46

Combine if statements

3405415

Import type

0ff2e01

MicroFish91 marked this pull request as ready for review September 11, 2023 22:50

MicroFish91 requested a review from a team as a code owner September 11, 2023 22:50

MicroFish91 commented Sep 11, 2023

View reviewed changes

Update test display

d689816

bwateratmsft reviewed Sep 12, 2023

View reviewed changes

Misc

a721f97

MicroFish91 force-pushed the mwf/deployWorkspaceProject-III branch from 6786c9c to a721f97 Compare September 12, 2023 21:20

Add additional parse notes

0f86bdc

MicroFish91 added 2 commits September 13, 2023 10:22

Update var name

4cab767

Merge branch 'mwf/deployWorkspaceProject-II' of https://github.com/mi…

fe4f955

…crosoft/vscode-azurecontainerapps into mwf/deployWorkspaceProject-III

nturinski reviewed Sep 13, 2023

View reviewed changes

Updates

de11fc0

MicroFish91 force-pushed the mwf/deployWorkspaceProject-III branch from 6ca6925 to de11fc0 Compare September 13, 2023 19:44

nturinski approved these changes Sep 13, 2023

View reviewed changes

Base automatically changed from mwf/deployWorkspaceProject-II to main September 15, 2023 18:53

MicroFish91 added 2 commits September 15, 2023 11:55

Merge with main

3a3942c

Update microsoft header

7a236f0

MicroFish91 merged commit d28f31b into main Sep 15, 2023

MicroFish91 deleted the mwf/deployWorkspaceProject-III branch September 15, 2023 19:04

microsoft locked and limited conversation to collaborators Mar 8, 2024

		@@ -0,0 +1,7 @@
		## Modified example from Azure-Samples/acr-build-helloworld-node
		FROM node:15-alpine


		import type { PortRange } from "../../extension.bundle";

		export interface MockIngressContext {


		context.dockerfileExposePorts = await getDockerfileExposePorts(context.dockerfilePath);

		if (context.alwaysPromptIngress) {

Conversation

MicroFish91 commented Sep 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicroFish91 Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MicroFish91 commented Sep 11, 2023 •

edited

Loading

MicroFish91 Sep 12, 2023 •

edited

Loading

MicroFish91 Sep 13, 2023 •

edited

Loading

MicroFish91 Sep 13, 2023 •

edited

Loading

MicroFish91 Sep 13, 2023 •

edited

Loading