Skip to content
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.

Sign up for GitHub

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

Jump to bottom

helloworld-go: install CA certs #1143

Merged
merged 2 commits into from
Apr 15, 2019
Merged

helloworld-go: install CA certs #1143

merged 2 commits into from
Apr 15, 2019

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Apr 4, 2019

All users who are querying any HTTPS endpoints will need this and won't understand why it's failing.

cc: @grayside

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 4, 2019
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 4, 2019
@RichieEscarez
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@ahmetb
Copy link
Contributor Author

ahmetb commented Apr 4, 2019

hmm there are many more occurrences, I'll probably broaden this PR.

install/Knative-with-Gloo.md:159:FROM alpine
serving/samples/helloworld-go/Dockerfile:17:FROM alpine
serving/samples/helloworld-go/README.md:78:   FROM alpine
serving/samples/secrets-go/Dockerfile:22:FROM alpine
serving/samples/secrets-go/README.md:110:   FROM alpine
community/samples/serving/helloworld-elixir/Dockerfile:23:FROM alpine:latest
community/samples/serving/helloworld-elixir/README.md:72:   FROM alpine:latest

@ahmetb
helloworld-go: install CA certs
11178de 
All users who are querying any HTTPS endpoints will need this and won't understand why it's failing.
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2019
grayside
grayside reviewed Apr 4, 2019
View reviewed changes
docs/serving/samples/secrets-go/Dockerfile Outdated Show resolved Hide resolved
docs/serving/samples/secrets-go/README.md Outdated Show resolved Hide resolved
grayside
grayside reviewed Apr 4, 2019
View reviewed changes
Copy link
Contributor

@grayside grayside left a comment

Choose a reason for hiding this comment

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

This needs some additional testing, I've heard that just apk --no-cache add ca-certificates does not work, but apk update && apk --no-cache --update add bash openssl-dev ca-certificates does. The answer is in the middle, because I'm sure BASH isn't a factor here.

@ahmetb
Copy link
Contributor Author

ahmetb commented Apr 9, 2019

I've heard that just apk --no-cache add ca-certificates does not work.

I need to see the repro. apk update increases the image size, and once you add it, --no-cache and --update are meaningless.

@samodell
Copy link
Contributor

@ahmetb Have we reached a decision on what command to add here?

@ahmetb
address fixes
13d4db5 
Signed-off-by: Ahmet Alp Balkan <[email protected]>
@ahmetb
Copy link
Contributor Author

ahmetb commented Apr 15, 2019

@samodell I think it's good, I'll leave it to @grayside to decide.

grayside
grayside reviewed Apr 15, 2019
View reviewed changes
Copy link
Contributor

@grayside grayside left a comment

Choose a reason for hiding this comment

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

(Non-blocker) For helloworld samples we try to explain the significance of each line. E.g.,

Alpine requires additional packages for outbound HTTPS requests.

The solution looks like what I would expect to work but a quick test would be helpful confirmation.

@samodell samodell mentioned this pull request Apr 15, 2019
Closed
@samodell
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@knative-prow-robot knative-prow-robot merged commit 3f8fbb2 into knative:master Apr 15, 2019
@samodell samodell mentioned this pull request Apr 17, 2019
Merged
samodell pushed a commit to samodell/docs that referenced this pull request Apr 23, 2019
@ahmetb @samodell
helloworld-go: install CA certs (knative#1143)
2e4182f 
* helloworld-go: install CA certs

All users who are querying any HTTPS endpoints will need this and won't understand why it's failing.

* address fixes

Signed-off-by: Ahmet Alp Balkan <[email protected]>
@samodell samodell mentioned this pull request Apr 24, 2019
Merged
knative-prow-robot pushed a commit that referenced this pull request Apr 24, 2019
@samodell @knative-prow-robot
Cherry-pick batch (#1232)
b644431 
* Knative with Gardener - Fix Installing Istio Typo (#1027)

* Update installation documentation with Minishift (#1114)

* Remove usage of kubectl. Using oc instead

* Update webhook admissions

* Mention IKS's one-click install process (#1131)

Signed-off-by: Doug Davis <[email protected]>

* helloworld-go: install CA certs (#1143)

* helloworld-go: install CA certs

All users who are querying any HTTPS endpoints will need this and won't understand why it's failing.

* address fixes

Signed-off-by: Ahmet Alp Balkan <[email protected]>

* Use istio-ingressgateway servce as this is the new one used in Knative 0.3 and above (#1171)

* knative.slack.com -> slack.knative.dev (#1176)

* Update Knative-custom-install.md (#1178)

change the name `release.yaml` to `build.yaml`

* fix README (#1180)

Replace pubspec field `private` with `publish_to`.  [ref](https://www.dartlang.org/tools/pub/pubspec#publish_to)

See @52f464f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grayside grayside grayside approved these changes

@RichieEscarez RichieEscarez Awaiting requested review from RichieEscarez

@samodell samodell Awaiting requested review from samodell

Assignees

@RichieEscarez RichieEscarez

@samodell samodell

Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ahmetb @RichieEscarez @knative-prow-robot @samodell @grayside @googlebot