serving/samples: remove EXPOSE directive from helloworld #644

grayside · 2018-12-07T23:38:51Z

Remove EXPOSE directive from all helloworld serving samples.

@steren indicated this was not required, so in the interest of simplicity lets remove it.

steren · 2018-12-07T23:39:48Z

serving/samples/helloworld-dart/README.md

   FROM google/dart-runtime

   # Configure and document the service HTTP port.
   ENV PORT 8080


You should remove PORT, this is injected by Knative.

(true for all samples)

I prefer to keep the explicit default as a way of documenting the contract and making it obvious how to override the port for experimentation.

8080 is not part of the runtime contract, only PORT is: https://github.com/knative/serving/blob/master/docs/runtime-contract.md#process

Explicitly showing this remains valuable, even if the value is a common convention rather than an official default.

Just to make sure we have the same definition here, the ENV directive is a way to set a default environment variable value at build time which is overridden at run time by any mechanism which injects environment variables into the container.

I see including this line as a way of:

Having sensible defaults to ensure the container will work locally.

Documenting what the PORT env var is about for practitioners that read the Dockerfile but not the docs.

My concerns is that users will assume these are the default values of PORT.

As far as I understand, the ENV directive is not needed for running locally when using docker -p.

Running locally is better done using $ PORT=8080 docker run -p 8080:${PORT} -e PORT=${PORT} [IMAGE]

This is setting a default value for the service, which is a good practice. It also allows the container to be spun up with a more typical and abbreviated command:

docker run -p 8080 [IMAGE]

A good amount of the value in this is if developers take the Dockerfile and pivot to using this as part of an ongoing service.

The Day 1 value is in surfacing a bit of the runtime contract for developers that look at code first. What if a comment were added indicating that 8080 is not a requirement but a sane default?

OK to leave ENV PORT 8080 in the Dockerfiles, but indeed please change the comment, "Configure the service HTTP port." is not correct. The service HTTP port is not configured via this env var. Use something like "Service must listen on $PORT, set value for local development"

# Service must listen to $PORT environment variable. # This default value facilitates local development.

averikitsch · 2018-12-19T19:32:47Z

serving/samples/helloworld-clojure/Dockerfile

 RUN mv "$(lein uberjar | sed -n 's/^Created \(.*standalone\.jar\)/\1/p')" app-standalone.jar

-# Configure and document the service HTTP port.
+# Configure the service HTTP port.


Suggested change

# Configure the service HTTP port.

# Set the default value for PORT environment variable.

Should this change be consistent across all the samples?

Correct, just trying to figure out the phrasing.

averikitsch · 2019-01-03T21:00:11Z

serving/samples/helloworld-clojure/README.md

   RUN mv "$(lein uberjar | sed -n 's/^Created \(.*standalone\.jar\)/\1/p')" app-standalone.jar

-   # Configure and document the service HTTP port.
+   # Configure the service HTTP port.


Can you update the ReadMes as well?

Working on it. This seems like a good spot for automation.

knative-prow-robot · 2019-01-11T18:25:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: averikitsch, grayside

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [averikitsch]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

averikitsch · 2019-01-11T18:25:51Z

/lgtm

averikitsch · 2019-01-11T18:55:11Z

/test pull-knative-docs-build-tests

averikitsch · 2019-01-11T21:22:54Z

I0111 18:56:10.123] FILE: serving/samples/helloworld-dart/README.md
I0111 18:56:10.482] [✖] www.dartlang.org
I0111 18:56:10.482] 
I0111 18:56:10.483] ERROR: dead links found!
I0111 18:56:10.737] 
I0111 18:56:10.737] FILE: serving/samples/helloworld-elixir/README.md
I0111 18:56:11.025] [✖] http://localhost:4000
I0111 18:56:11.026] 
I0111 18:56:11.026] ERROR: dead links found!

Tests error at dead links; however they aren't dead.
@mattmoor @samodell any insight?

samodell · 2019-01-11T22:18:02Z

It looks like our new build tests are a bit over-sensitive. @adrcunha

Can we customize the checker so that it ignores links that point to http://localhost... ?
I'm less sure why www.dartlang.org isn't recognized as a valid link. Does the checker require "http://"?

adrcunha · 2019-01-11T23:43:20Z

Probably the checker doesn't like an address without protocol, and I'm pretty sure localhost:4000 is not returning content at the time of checking. :)

adrcunha · 2019-01-12T00:14:46Z

Actually the checker is correct: you must add the protocol to the darlang.org link, otherwise it will be treated as a "local" file. Just try clicking it in the README file and you'll see a nice 404 GitHub page.

averikitsch · 2019-01-18T17:40:47Z

/test pull-knative-docs-build-tests

samodell · 2019-01-18T17:46:44Z

It looks like it's failing because of the merge conflicts. That test should pass once those are cleared up.

samodell · 2019-01-24T00:31:57Z

@grayside Ping to resolve merge conflicts. This should be mergeable after that.

averikitsch · 2019-01-24T18:08:57Z

/lgtm

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 7, 2018

knative-prow-robot requested review from mattmoor and mchmarny December 7, 2018 23:38

steren reviewed Dec 7, 2018

View reviewed changes

samodell added kind/enhancement triage/needs-eng-input Engineering input is requested labels Dec 11, 2018

RichieEscarez assigned averikitsch Dec 19, 2018

averikitsch suggested changes Dec 19, 2018

View reviewed changes

grayside added 2 commits January 3, 2019 11:12

serving/samples: remove EXPOSE directive from helloworld

0990f51

serving/samples: update ENV PORT comment for clarified purpose

ee93f69

grayside force-pushed the helloworld/remove-expose branch from 00bb321 to ee93f69 Compare January 3, 2019 19:27

averikitsch reviewed Jan 3, 2019

View reviewed changes

serving/samples: update README for Dockerfile changes

6027abe

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 3, 2019

Merge branch 'master' into helloworld/remove-expose

486478c

averikitsch approved these changes Jan 11, 2019

View reviewed changes

knative-prow-robot added the approved label Jan 11, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2019

samodell mentioned this pull request Jan 11, 2019

New markdown link checker and linter are overly sensitive #750

Closed

Merge branch 'master' into helloworld/remove-expose

af3daf8

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 24, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2019

knative-prow-robot merged commit 921aa48 into knative:master Jan 24, 2019

	# Configure the service HTTP port.
	# Set the default value for PORT environment variable.

serving/samples: remove EXPOSE directive from helloworld #644

serving/samples: remove EXPOSE directive from helloworld #644

Uh oh!

Conversation

grayside commented Dec 7, 2018

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!

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!

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!

knative-prow-robot commented Jan 11, 2019

Uh oh!

averikitsch commented Jan 11, 2019

Uh oh!

averikitsch commented Jan 11, 2019

Uh oh!

averikitsch commented Jan 11, 2019

Uh oh!

samodell commented Jan 11, 2019

Uh oh!

adrcunha commented Jan 11, 2019

Uh oh!

adrcunha commented Jan 12, 2019

Uh oh!

averikitsch commented Jan 18, 2019

Uh oh!

samodell commented Jan 18, 2019

Uh oh!

samodell commented Jan 24, 2019

Uh oh!

averikitsch commented Jan 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants