Update README. #414

MichaelRybak · 2020-04-30T00:14:36Z

No description provided.

MichaelRybak · 2020-04-30T00:16:09Z

README.md

@@ -31,13 +31,18 @@ own and can obtain certificates for.

 ##### Manual installation

-  1. Install Go version 1.10 or higher. Optionally, set
+  1. Install Go version 1.10. Optionally, set


Google will validate all contributions with Travis running Go 1.10, so should we suggest installing specifically this version?

I've installed Go 1.14, used strings.ReplaceAll (that's not available in Go 1.10) and got a build error from Travis:
https://travis-ci.org/github/ampproject/amppackager/builds/680776394

@twifkak perhaps it's time to update travis.yml? What do you think?

I wouldn't remove the "or higher" unless there's a known breakage at higher versions. Don't want to make installation unnecessarily hard. I guess I have to exclude -mod=vendor from that rule. :)

Re: updating travis.yml, SGTM. It may even be possible to test on multiple versions? e.g. https://github.com/streadway/amqp/blob/master/.travis.yml

MichaelRybak · 2020-04-30T00:19:33Z

README.md

     [$GOPATH](https://github.com/golang/go/wiki/GOPATH) to something (default
     is `~/go`) and/or add `$GOPATH/bin` to `$PATH`.
-  2. `go get -u -mod=vendor github.com/ampproject/amppackager/cmd/amppkg`
+  1. `go get -u github.com/ampproject/amppackager/cmd/amppkg`


-mod=vendor is not available in Go 1.14: GoogleContainerTools/skaffold#3699

I've tested it today with Go 1.10 and it didn't work for me either (but it did work when I removed -mod=vendor). Here's the error I got:

michaelrybak$ go get -u -mod=vendor github.com/ampproject/amppackager/cmd/amppkg flag provided but not defined: -mod usage: get [-d] [-f] [-fix] [-insecure] [-t] [-u] [-v] [build flags] [packages] Run 'go help get' for details.

@twifkak, do we still need to generate the vendor directory?

Oof. They're not making it easy. I guess we should list two command lines: with -mod=vendor if 1.10-1.13, and without if 1.14+. I think it's too soon to up our minimum version to 1.14 -- e.g. it's not even in the Ubuntu release from 7 days ago: https://packages.ubuntu.com/focal/golang-any, let alone in the previous LTS version.

We could just do as you've suggested, which AFAIK means that the vendor directory will be ignored on Go 1.10-1.13. I'm not wild about that, for two reasons:

I think it's slow? Involves a lot of git clones.

I'm just not knowledgeable enough in how go modules work. After doing all those git clones, does Go verify against go.sum? I would prefer some safety check that users are getting the version of the libs that we tested against.

I could easily be convinced on both fronts.

(To make things more confusing, it seems the official documentation is incorrect. https://github.com/golang/go/wiki/Modules#how-do-i-use-vendoring-with-modules-is-vendoring-going-away and https://golang.org/doc/go1.14#go-command seem to suggest continued support for -mod=vendor. But the commit that you link to is unequivocal.)

MichaelRybak · 2020-04-30T00:20:30Z

README.md

-     --enable-features=SignedHTTPExchange
-     'data:text/html,<a href="https://localhost:8080/priv/doc/https://amppackageexample.com/">click me'
+     alias chrome = [FULL PATH TO CHROME BINARY]
+     PATH_TO_FULLCHAIN_PEM = [FULL PATH TO fullchain.pem]


I didn't notice "path/to/fullchain.pem" template in the command below:

--ignore-certificate-errors-spki-list=$(openssl x509 -pubkey -noout -in path/to/fullchain.pem

Added a dedicated variable PATH_TO_FULLCHAIN_PEM so the requirement to insert the user cert path becomes explicit.

MichaelRybak · 2020-04-30T00:22:26Z

README.md

+     alias chrome = [FULL PATH TO CHROME BINARY]
+     PATH_TO_FULLCHAIN_PEM = [FULL PATH TO fullchain.pem]
+     chrome --user-data-dir=/tmp/udd\
+         --ignore-certificate-errors-spki-list=$(\


Added line breaks for readability.

banaag · 2020-04-30T00:54:46Z

The changes look reasonable to me but you should wait for twifkak@ to bless.

twifkak · 2020-04-30T22:32:40Z

README.md

@@ -31,13 +31,18 @@ own and can obtain certificates for.

 ##### Manual installation

-  1. Install Go version 1.10 or higher. Optionally, set
+  1. Install Go version 1.10. Optionally, set


I wouldn't remove the "or higher" unless there's a known breakage at higher versions. Don't want to make installation unnecessarily hard. I guess I have to exclude -mod=vendor from that rule. :)

Re: updating travis.yml, SGTM. It may even be possible to test on multiple versions? e.g. https://github.com/streadway/amqp/blob/master/.travis.yml

twifkak · 2020-04-30T22:35:44Z

README.md

     [$GOPATH](https://github.com/golang/go/wiki/GOPATH) to something (default
     is `~/go`) and/or add `$GOPATH/bin` to `$PATH`.
-  2. `go get -u -mod=vendor github.com/ampproject/amppackager/cmd/amppkg`
+  1. `go get -u github.com/ampproject/amppackager/cmd/amppkg`


Oof. They're not making it easy. I guess we should list two command lines: with -mod=vendor if 1.10-1.13, and without if 1.14+. I think it's too soon to up our minimum version to 1.14 -- e.g. it's not even in the Ubuntu release from 7 days ago: https://packages.ubuntu.com/focal/golang-any, let alone in the previous LTS version.

We could just do as you've suggested, which AFAIK means that the vendor directory will be ignored on Go 1.10-1.13. I'm not wild about that, for two reasons:

I think it's slow? Involves a lot of git clones.

I'm just not knowledgeable enough in how go modules work. After doing all those git clones, does Go verify against go.sum? I would prefer some safety check that users are getting the version of the libs that we tested against.

I could easily be convinced on both fronts.

(To make things more confusing, it seems the official documentation is incorrect. https://github.com/golang/go/wiki/Modules#how-do-i-use-vendoring-with-modules-is-vendoring-going-away and https://golang.org/doc/go1.14#go-command seem to suggest continued support for -mod=vendor. But the commit that you link to is unequivocal.)

twifkak · 2020-04-30T22:51:41Z

README.md

-     --enable-features=SignedHTTPExchange
-     'data:text/html,<a href="https://localhost:8080/priv/doc/https://amppackageexample.com/">click me'
+     alias chrome = [FULL PATH TO CHROME BINARY]
+     PATH_TO_FULLCHAIN_PEM = [FULL PATH TO fullchain.pem]


twifkak · 2020-04-30T22:51:45Z

README.md

+     alias chrome = [FULL PATH TO CHROME BINARY]
+     PATH_TO_FULLCHAIN_PEM = [FULL PATH TO fullchain.pem]
+     chrome --user-data-dir=/tmp/udd\
+         --ignore-certificate-errors-spki-list=$(\


MichaelRybak · 2020-05-07T19:36:28Z

@twifkak @banaag Devin, Allan, I've removed "mod=vendor" and "1.10 or higher" updates - could you re-LGTM the remaining changes? We can have this PR pulled in, and update the version/modules stuff later.

twifkak · 2020-05-07T19:51:09Z

README.md

@@ -34,10 +34,15 @@ own and can obtain certificates for.
  1. Install Go version 1.10 or higher. Optionally, set
     [$GOPATH](https://github.com/golang/go/wiki/GOPATH) to something (default
     is `~/go`) and/or add `$GOPATH/bin` to `$PATH`.
-  2. `go get -u -mod=vendor github.com/ampproject/amppackager/cmd/amppkg`
+  1. `go get -u -mod=vendor github.com/ampproject/amppackager/cmd/amppkg`


(optional) For now, I'd be OK with putting an if statement here based on go version, until we work out the details.

twifkak

Thanks!

Update setup instructions in README.md.

Update README.

46268ea

googlebot added the cla: yes label Apr 30, 2020

MichaelRybak commented Apr 30, 2020

View reviewed changes

MichaelRybak marked this pull request as ready for review April 30, 2020 00:28

MichaelRybak requested review from twifkak and banaag April 30, 2020 00:48

twifkak requested changes Apr 30, 2020

View reviewed changes

Revert debatable updates.

4bd194a

MichaelRybak requested a review from twifkak May 7, 2020 19:37

twifkak approved these changes May 7, 2020

View reviewed changes

MichaelRybak added 3 commits May 7, 2020 12:58

Add if statement for mod=vendor

b1ad962

Upd

f0b49fb

Upd

03f487b

twifkak approved these changes May 7, 2020

View reviewed changes

Upd

92a28b1

banaag approved these changes May 7, 2020

View reviewed changes

MichaelRybak merged commit 106947c into ampproject:master May 7, 2020

MichaelRybak deleted the setup4 branch May 15, 2020 17:52

cpapazian pushed a commit to cpapazian/amppackager that referenced this pull request May 19, 2020

Update README. (ampproject#414)

c8e9c4a

Update setup instructions in README.md.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update README. #414

Update README. #414

MichaelRybak commented Apr 30, 2020

MichaelRybak Apr 30, 2020 •

edited

Loading

banaag Apr 30, 2020

twifkak Apr 30, 2020

MichaelRybak Apr 30, 2020 •

edited

Loading

banaag Apr 30, 2020

twifkak Apr 30, 2020

MichaelRybak Apr 30, 2020 •

edited

Loading

twifkak Apr 30, 2020

MichaelRybak Apr 30, 2020

twifkak Apr 30, 2020

banaag commented Apr 30, 2020

twifkak Apr 30, 2020

twifkak Apr 30, 2020

twifkak Apr 30, 2020

twifkak Apr 30, 2020

MichaelRybak commented May 7, 2020 •

edited

Loading

twifkak May 7, 2020

MichaelRybak May 7, 2020

twifkak left a comment

Update README. #414

Update README. #414

Conversation

MichaelRybak commented Apr 30, 2020

MichaelRybak Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRybak Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRybak Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banaag commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRybak commented May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twifkak left a comment

Choose a reason for hiding this comment

MichaelRybak Apr 30, 2020 •

edited

Loading

MichaelRybak Apr 30, 2020 •

edited

Loading

MichaelRybak Apr 30, 2020 •

edited

Loading

MichaelRybak commented May 7, 2020 •

edited

Loading