-
Notifications
You must be signed in to change notification settings - Fork 195
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
Auto detect port from Dockerfile #4454
base: main
Are you sure you want to change the base?
Conversation
Hi, @saragluna , @weikanglim , @puicchan. |
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.
@rujche Thanks for getting this started! I left two blocking comments:
- Move the logic into
appdetect
and surface metadata that way - Make sure to handle the full EXPOSE syntax
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
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.
Thanks @rujche ! This is looking better, thanks for adopting the suggestion to move the detection logic into appdetect
package.
I left some comments for the next review iteration.
type Docker struct { | ||
Path string | ||
ExposedPorts map[int]string | ||
} |
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 do prefer this being in appdetect.go
. Docker
is an exposed contract of the appdetect
package/library. docker.go
is an internal implementation of analyzing docker that isn't exposed. If we think along those lines, I think it makes more sense to have contract files in appdetect.go
and not in docker.go
.
}, nil | ||
} | ||
} | ||
|
||
return nil, nil | ||
} | ||
|
||
func getExposedPorts(dockerfilePath string) (map[int]string, error) { |
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.
Let's think about breaking down this functionality differently. getExposedPorts
right now takes the entire file, I think we could step down into a helper function that takes a line instead. I like the idea of parsePorts(s string) ([]port, error)
.
The way I'm thinking about this: If we had to parse a different line of syntax, let's say, RUN ...
, it wouldn't be easy to implement this additional feature without introducing additional changes. Breaking it down in the right way will also help us provide unit test coverage over the algorithm we care about: port parsing, without creating heavyweight tests that exercise the full functionality. Each test case is just a different input string, instead of a different input file.
My recommendations would be something along the following lines:
- In
detectDocker
, let's open the file and close it for reading. - We read line by line using the scanner
- If
line
has prefixEXPOSE
-- we know it's an expose directive. We invokeparsePorts(line[len("EXPOSE"):)
to parse the ports in the remainder of the line.
Note that len("EXPOSE")
will be optimized by the compiler into a constant, but if we feel that the code is too verbose, we could also parse the first WORD / Directive from the line -- but I would recommend to keep the program simple at this point.
|
||
type Docker struct { | ||
Path string | ||
ExposedPorts map[int]string |
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'm okay with introducing a struct here for type Port struct { Number int Protocol string }
"strings" | ||
) | ||
|
||
func detectDocker(path string, entries []fs.DirEntry) (*Docker, error) { | ||
for _, entry := range entries { | ||
if strings.ToLower(entry.Name()) == "dockerfile" { | ||
dockerFilePath := filepath.Join(path, entry.Name()) | ||
exposedPorts, _ := getExposedPorts(dockerFilePath) |
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.
Let's do propagate the err
returned here. In general, if we are trying to suppress errors, we can consider adding a comment on why this suppression is necessary.
Imagine if the filesystem had a temporary disk error reading the file temporarily. We want the detection to halt because of that, instead of saying "I couldn't find ports" -- which on the next re-run, would produce a different set of results.
if svc.Language == appdetect.Java { | ||
serviceSpec.Port = 8080 | ||
} | ||
} else { | ||
ports := svc.Docker.ExposedPorts |
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.
Just leaving this as a note, when you get to this requirement:
If more than one port is exposed, prompt for a selection (console.Select) with the multiple ports available, and an "Other" selection to provide the port number.
Feel free to ping me directly on Teams if you have any questions here.
"testing" | ||
) | ||
|
||
func TestDetectPortInDockerfile(t *testing.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.
Let's revisit how this test should look like, once we think about testing parsing a string instead of an entire file.
I'd recommend taking full advantage of go's table-driven tests to concisely represent the different test cases. Here's an example taking from scaffold/funcs_test.go
, I'd imagine that we would be able to structure it similarly.
func Test_containerAppName(t *testing.T) {
tests := []struct {
name string
in string
want string
}{
{"allowed characters", "MyApp_!#%^", "myapp"},
{"dash at front or end", "-my-app-", "my-app"},
{"multiple dashes", "my----app", "my-app"},
{"at length", "123456789app", "123456789app"},
{"over length", "123456789my-app", "123456789my"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
actual := containerAppName(tt.in, 12)
assert.Equal(t, tt.want, actual)
})
}
}
|
||
ports := make(map[int]string) | ||
scanner := bufio.NewScanner(file) | ||
exposeRegex := regexp.MustCompile(`^EXPOSE\s+(.+)$`) |
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.
Let's avoid using regex here if we can. Some ideas from the other comment.
Resolve #4443
Auto detect port from Dockerfile