service/samples: add .dockerignore to samples with local build disruption risk by grayside · Pull Request #1392 · knative/docs

grayside · 2019-05-29T22:46:30Z

Problem

If someone runs a local build in these samples, it will generate files that will make their way into the container. Once there, these files can interfere with the build.
Files unrelated to the application itself such as the Dockerfile or README.md can excessively reset the layer cache.

Proposed Changes

Add a new .dockerignore for serving samples subject to (1), with language-specific exclusions of generated files. This includes: Python, C#, PHP, Nodejs
Add Dockerfile and README.md for (2)
Include a new step in the README after the Dockerfile is set up to note creating the .dockerignore.

…d interference

grayside · 2019-05-29T22:47:07Z

/cc @steren

RichieEscarez · 2019-05-29T23:04:58Z

the _index.md file in each folder is for the website build, does that need to be ignored too?

grayside · 2019-05-29T23:12:34Z

That's a fantastic question Richie. In the interests of making this a light touch (it borders on going too far for a helloworld/quickstart), my take is that we should not include _index.md or other elements of the knative/docs repo itself.

If we find there are real performance gains to be had, we can patch in some additional .dockerignore lines as part of the CI process.

sebgoa · 2019-05-30T07:42:58Z

   CMD ["dotnet", "out/helloworld-csharp.dll"]
   ```

+1. Add a `.dockerignore` file to ensure local builds cannot disrupt container builds.


Why tell folks to add a .dockerignore file since you added it already. If they clone the source code they will have the file already there.

We don't tell them to clone the source code, we tell them to assemble the files through CLI scaffolding and copy & paste.

Suggested change

1. Add a `.dockerignore` file to ensure local builds cannot disrupt container builds.

1. Create a `.dockerignore` file to ensure that any files related to a local build, do not affect the container that you build for deployment.

Can we make this change throughout?

RichieEscarez

One global change requested, otherwise looks good to me.

grayside · 2019-05-30T20:05:42Z

One global change applied.

RichieEscarez

/approve

Looking for an LGTM from a SME, please

knative-prow-robot · 2019-05-30T20:09:23Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [RichieEscarez]

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

evankanderson

/lgtm
/hold

One question on grammar that I'm not sure about, and a request whether technical assistance is available.

evankanderson · 2019-05-30T20:13:06Z


+1. Create a `.dockerignore` file to ensure that any files related to a local build, do not affect the container that you build for deployment.
+
+   ```ignore


@RichieEscarez

Is there any way to do some fancy server-side include magic here rather than needing to repeat the content?

for the rendered content on the site, yes we have a few options (ie. using Hugo variables to inject chunks of content). the downside is that when the content is viewed in GitHub, its no longer legible (more difficult for authors or one-off contributors, especially those who use the GitHub WYSIWYG UI). im toying with the idea of getting our Netlify builds to run on demand (therefore the authoring process / PR review and merge steps would include running a "test build" to render and verify the output) but that too adds to the overhead of contributing. I think the first place we want to start to implement "content reuse" and "single sourcing" are for the multiple install guides

we should chat more if you have ideas here?

for the rendered content on the site, yes we have a few options (ie. using Hugo variables to inject chunks of content). the downside is that when the content is viewed in GitHub, its no longer legible (more difficult for authors or one-off contributors, especially those who use the GitHub WYSIWYG UI). im toying with the idea of getting our Netlify builds to run on demand (therefore the authoring process / PR review and merge steps would include running a "test build" to render and verify the output) but that too adds to the overhead of contributing. I think the first place we want to start to implement "content reuse" and "single sourcing" are for the multiple install guides

we should chat more if you have ideas here?

I would like something like SSI Include syntax:



Which seems like it would be fairly natural to read. But I don't know if Hugo has that capability.

Co-Authored-By: RichieEscarez <rescarez@google.com>

Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com>

googlebot · 2019-05-30T23:05:58Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

grayside · 2019-05-30T23:06:44Z

Comma extraction is complete. Thanks for using the suggestion feature!

evankanderson · 2019-05-31T00:20:18Z

/lgtm

googlebot · 2019-05-31T00:20:40Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

evankanderson · 2019-05-31T00:20:46Z

/hold cancel

…tion risk (knative#1392) * service/samples: add .dockerignore to samples with risk of local build interference * serving/samples: .dockerignore readme text update * Update docs/serving/samples/hello-world/helloworld-csharp/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-nodejs/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-python/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-php/README.md Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com>

* pr#1408 * pr#1398 * service/samples: add .dockerignore to samples with local build disruption risk (#1392) * service/samples: add .dockerignore to samples with risk of local build interference * serving/samples: .dockerignore readme text update * Update docs/serving/samples/hello-world/helloworld-csharp/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-nodejs/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-python/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-php/README.md Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com> * Fix the wrong information in grpc example (#1380) * pr#1363 * Fix samples path (#1357) * Copy the updates from knative/eventing#1241. (#1355) * add reference to custom DNS config (#1327) * add reference to custom DNS config * Update docs/install/getting-started-knative-app.md Co-Authored-By: Sam O'Dell <31352624+samodell@users.noreply.github.com> * fix relative link * Fix PR#1202 * Document serving tag resolution (#1260) Our controller does resolution of tags to digests, which has been a source of confusion. This documents the fact that we do it, why we do it, and how to configure the controller to work around common issues.

service/samples: add .dockerignore to samples with risk of local buil…

7d7fa99

…d interference

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

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 29, 2019

knative-prow-robot requested review from RichieEscarez and samodell May 29, 2019 22:46

knative-prow-robot requested a review from steren May 29, 2019 22:47

sebgoa reviewed May 30, 2019

View reviewed changes

RichieEscarez suggested changes May 30, 2019

View reviewed changes

serving/samples: .dockerignore readme text update

0d4f398

RichieEscarez approved these changes May 30, 2019

View reviewed changes

knative-prow-robot added the approved label May 30, 2019

RichieEscarez added the cherrypick-0.6 label May 30, 2019

evankanderson reviewed May 30, 2019

View reviewed changes

knative-prow-robot assigned evankanderson May 30, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 30, 2019

RichieEscarez reviewed May 30, 2019

View reviewed changes

Comment thread docs/serving/samples/hello-world/helloworld-csharp/README.md Outdated

RichieEscarez reviewed May 30, 2019

View reviewed changes

Comment thread docs/serving/samples/hello-world/helloworld-nodejs/README.md Outdated

RichieEscarez reviewed May 30, 2019

View reviewed changes

Comment thread docs/serving/samples/hello-world/helloworld-python/README.md Outdated

Update docs/serving/samples/hello-world/helloworld-csharp/README.md

df2d8b4

Co-Authored-By: RichieEscarez <rescarez@google.com>

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

grayside and others added 3 commits May 30, 2019 16:05

Update docs/serving/samples/hello-world/helloworld-nodejs/README.md

39c1211

Co-Authored-By: RichieEscarez <rescarez@google.com>

Update docs/serving/samples/hello-world/helloworld-python/README.md

b0f9d60

Co-Authored-By: RichieEscarez <rescarez@google.com>

Update docs/serving/samples/hello-world/helloworld-php/README.md

e607e5e

Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com>

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels May 30, 2019

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

evankanderson added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels May 31, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2019

knative-prow-robot merged commit 0d5b492 into knative:master May 31, 2019

RichieEscarez mentioned this pull request Jun 6, 2019

Cherry pick changes from master into 0.6 #1423

Merged

RichieEscarez removed the cherrypick-0.6 label Jun 6, 2019

	1. Add a `.dockerignore` file to ensure local builds cannot disrupt container builds.
	1. Create a `.dockerignore` file to ensure that any files related to a local build, do not affect the container that you build for deployment.


		1. Create a `.dockerignore` file to ensure that any files related to a local build, do not affect the container that you build for deployment.

		```ignore

Conversation

grayside commented May 29, 2019

Problem

Proposed Changes

Uh oh!

grayside commented May 29, 2019

Uh oh!

RichieEscarez commented May 29, 2019

Uh oh!

grayside commented May 29, 2019

Uh oh!

sebgoa May 30, 2019

Choose a reason for hiding this comment

Uh oh!

grayside May 30, 2019

Choose a reason for hiding this comment

Uh oh!

RichieEscarez May 30, 2019

Choose a reason for hiding this comment

Uh oh!

RichieEscarez left a comment

Choose a reason for hiding this comment

Uh oh!

grayside commented May 30, 2019

Uh oh!

RichieEscarez left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 30, 2019

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson May 30, 2019

Choose a reason for hiding this comment

Uh oh!

RichieEscarez May 30, 2019 • 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!

evankanderson May 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented May 30, 2019

Uh oh!

grayside commented May 30, 2019

Uh oh!

evankanderson commented May 31, 2019

Uh oh!

googlebot commented May 31, 2019

Uh oh!

evankanderson commented May 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RichieEscarez May 30, 2019 •

edited

Loading