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.

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

Add autocert and nacl #1698

Merged
merged 1 commit into from
Dec 5, 2017
Merged

Add autocert and nacl #1698

merged 1 commit into from
Dec 5, 2017

Conversation

kevinburke
Copy link
Contributor

Autocert is a Go maintained project that facilitates provisioning TLS
certificates and starting a server using those certificates.

kevinburke/nacl implements all of the NaCL API's in Go, in a way
that's compatible with the C library offered here:
https://nacl.cr.yp.to.

Please check if what you want to add to awesome-go list meets quality standards before sending pull request. Thanks!

Please provide package links to:

Note: that new categories can be added only when there are 3 packages or more.

Make sure that you've checked the boxes below before you submit PR:

  • I have added my package in alphabetical order.
  • I have an appropriate description with correct grammar.
  • I know that this package was not listed before.
  • I have added godoc link to the repo and to my pull request.
  • I have added coverage service link to the repo and to my pull request.
  • I have added goreportcard link to the repo and to my pull request.
  • I have read Contribution guidelines, maintainers note and Quality standard.

Autocert is a Go maintained project that facilitates provisioning TLS
certificates and starting a server using those certificates.

kevinburke/nacl implements all of the NaCL API's in Go, in a way
that's compatible with the C library offered here:
https://nacl.cr.yp.to.
@kevinburke
Copy link
Contributor Author

This was previously submitted as #1630, I got the following response:

Please provide autocert links. Also take a loook on nacl goreportcard and fix some errors.

I added links. I don't, um, think the comments on goreportcard are accurate or helpful, let's walk through them:

  1. It's reporting this problem:
error: ExampleLoad refers to unknown identifier: Load (vet)

This is confusing, because there is an identifier named Load in that package, and godoc handles it with no problems; it looks like there's an error in the script generating the report card recommendation.

Indeed, running go vet ./... reports no errors, so I'm not sure how this is generating this error.

(master) $ go vet ./...
(master) $
  1. "golint" also warns about naming problems in an internal package. I copied this package from golang.org/x/crypto/ed25519/internal, and it's impossible for anyone else to use it, so I'm reluctant to change the names and get out of sync with the parent package.

  2. "ineffassign" warns about problems in an internal package copied directly from the golang.org/x/crypto/ed25519 source, that no one else can use. This ineffective assignments were copied directly from the C source code for SUPERCOP, e.g. the source code was taken directly from there. I've attempted to ask both the maintainer of the C code and the maintainer of the Go port about the assignment statements and unfortunately failed to get a response. I don't think it affects the quality of the underlying code in a meaningful way.

Copy link
Collaborator

@cassiobotaro cassiobotaro left a comment

Choose a reason for hiding this comment

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

@kevinburke where's autocert url's(doc, coverage, etc)?

Copy link
Contributor

@felipeweb felipeweb left a comment

Choose a reason for hiding this comment

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

LGTM

@avelino avelino merged commit 4ef3e30 into avelino:master Dec 5, 2017
@kevinburke kevinburke deleted the add-nacl branch August 3, 2018 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants