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

Use x crypto instead of sodium #2034

Merged
merged 5 commits into from
Aug 10, 2021

Conversation

andersjanmyr
Copy link
Contributor

The example that is using libsodium is hard to cross compile.
Here's an alternative example using google.com/x/crypto instead.

Perhaps it should be added as an alternative file instead of change to the current one.

@google-cla google-cla bot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Aug 9, 2021
@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #2034 (4d9b226) into master (54ec837) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2034   +/-   ##
=======================================
  Coverage   97.87%   97.87%           
=======================================
  Files         107      107           
  Lines        6907     6907           
=======================================
  Hits         6760     6760           
  Misses         81       81           
  Partials       66       66           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54ec837...4d9b226. Read the comment docs.

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 9, 2021

Perhaps it should be added as an alternative file instead of change to the current one.

Yes, please add a new example and distinguish between the two with pros/cons in a comment if you wish.

The libsodium example was a special request to see how it might be done, so I'm totally fine with adding a new example.
Thank you, @andersjanmyr !

@andersjanmyr andersjanmyr force-pushed the use_x_crypto_instead_of_sodium branch from d63d228 to b98fd4b Compare August 9, 2021 13:10
@andersjanmyr andersjanmyr force-pushed the use_x_crypto_instead_of_sodium branch from b98fd4b to 0fe4df8 Compare August 9, 2021 13:16
@andersjanmyr
Copy link
Contributor Author

@gmlewis I pushed it as a new directory, but I didn't change the name of the old directory.
Perhaps I should change the name of it to newreposecretwithlibsodium?

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Perhaps I should change the name of it to newreposecretwithlibsodium?

Sure, that sounds like a good idea.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @andersjanmyr !
LGTM.

If you want to rename the older libsodium example dir in this PR, that's fine with me.
Also, if you want to clean up any of the comment in that older example, that would be awesome too... (I see that you cleaned up some of the wording here... and maybe some of those cleanups also apply to the older example, but if not, that's OK too... thanks!)

@@ -0,0 +1,11 @@
module newreposecret
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
module newreposecret
module newreposecretwithxcrypto

@gmlewis gmlewis requested a review from wesleimp August 9, 2021 14:27
@andersjanmyr
Copy link
Contributor Author

Fixed the comments and modules.

@@ -1,13 +1,13 @@
// Copyright 2020 The go-github AUTHORS. All rights reserved.
// Copyright 2021 The go-github AUTHORS. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I wasn't clear before... any new file needs the current year it was created...
but any old file retains its original year which it was created (even if the file is renamed)...
so please revert this one line back to 2020. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 fixed.

@andersjanmyr andersjanmyr force-pushed the use_x_crypto_instead_of_sodium branch from 0aa4c1b to 4d9b226 Compare August 10, 2021 04:27
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @andersjanmyr !
LGTM.
Merging.

@gmlewis gmlewis merged commit 7b61934 into google:master Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants