Skip to content

Conversation

@grayside
Copy link
Contributor

@grayside grayside commented Sep 13, 2019

This adds a "broken" / difficult to troubleshoot sample for use in a troubleshooting tutorial.

This sample app is unusual in that it's is deliberately trying to show something not good as part of an exercise leading the developer through the full process of root cause analysis, fixing production, then improving the code.

Node.js (Canonical): GoogleCloudPlatform/nodejs-docs-samples#1480

@grayside grayside requested a review from tbpg September 13, 2019 21:12
@grayside grayside self-assigned this Sep 13, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 13, 2019
# See the License for the specific language governing permissions and
# limitations under the License.

# [START run_broken_dockerfile]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Dockerfile is using the version workshopped in knative/docs#1774, let's finalize for this repo here and I'll send a follow-up to port the other Cloud Run samples.

COPY . ./

# Build the binary.
RUN CGO_ENABLED=0 GOOS=linux go build -mod=readonly -v -o server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOS=linux is redundant i think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is redundant. Trying to decide if I want to lean in on explicit, add GOARCH, or rely on the defaults in the container and remove GOOS. Do you have thoughts here?


os.Setenv("TARGET", "")
brokenHandler(rr, req)
os.Setenv("NAME", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample demonstrates a brittle, low-visibility handling of an unset value, and a sane-defaults high visibility approach. I am testing the cases around both.

Copy link
Contributor

@tbpg tbpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but might want @broady to take another look if we're reusing the Dockerfile for other samples.


// [START run_broken_service]

// Sample hello demonstrates a difficult to troubleshoot service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comment would be nice here. What is troublesome about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sample is used in a tutorial where the reader is eventually given this answer, but a comment in the first code snippet would be counter-productive. Is there somewhere else I might add this detail?

@grayside
Copy link
Contributor Author

grayside commented Nov 5, 2019

@tbpg broady gave a verbal approval, merge at will.

@tbpg tbpg merged commit 59e2d5d into master Nov 6, 2019
@tbpg tbpg deleted the run/hello-broken branch November 6, 2019 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants