-
Notifications
You must be signed in to change notification settings - Fork 508
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
Don't output EXPOSE if empty port specified #490
Conversation
defaultPort: string | ||
} | ||
|
||
export function getExposeStatements(port: string): 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.
getExposeStatement? I don't see the reasoning behind the plural name.
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.
VS has scenarios where there are more than one (HTTPS and HTTP ports). I'm trying to keep similar to their logic.
}; | ||
|
||
function genDockerFile(serviceNameAndRelativePath: string, platform: string, os: string | undefined, port: string, { cmd, author, version, artifactName }: Partial<PackageInfo>): string { | ||
let exposeStatements = getExposeStatements(port); |
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.
There seems to be an inconsistency where sometimes the genDockerFile
has a 'port' parameter and sometimes it has an 'exposeStatements' parameter. Can we make that consistent?
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.
done
e35a8d2
to
39a35e4
Compare
configureWorkspace/configure.ts
Outdated
@@ -70,7 +77,14 @@ function genDockerFile(serviceNameAndRelativePath: string, platform: Platform, o | |||
let generators = generatorsByPlatform.get(platform); | |||
assert(generators, `Could not find dockerfile generator functions for "${platform}"`); | |||
if (generators.genDockerFile) { | |||
return generators.genDockerFile(serviceNameAndRelativePath, platform, os, port, { cmd, author, version, artifactName }); | |||
let contents = generators.genDockerFile(serviceNameAndRelativePath, platform, os, exposeStatements, { cmd, author, version, artifactName }); |
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.
Looks like you missed changing this 'exposeStatements' back to 'port'
Fixes #391
For next milestone.