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

Switch to x/crypto/pkcs12 instead of Azure repo #225

Merged
merged 1 commit into from
Oct 15, 2015

Conversation

paulmey
Copy link
Member

@paulmey paulmey commented Oct 12, 2015

No description provided.

@ahmetb
Copy link
Contributor

ahmetb commented Oct 13, 2015

Can you vendor with godep?

@paulmey
Copy link
Member Author

paulmey commented Oct 13, 2015

yup, one sec

@paulmey
Copy link
Member Author

paulmey commented Oct 13, 2015

done

@ahmetb
Copy link
Contributor

ahmetb commented Oct 13, 2015

Doesn't seem right. Did you use godep tool to vendor the pkg or just edited the json file? Source files are missing.

@paulmey
Copy link
Member Author

paulmey commented Oct 15, 2015

I used the tool, but forgot to git add the godeps workspace

@phinze
Copy link

phinze commented Oct 15, 2015

👍 Just confirmed that this PR fixes the Terraform master build, which was broken when Azure/go-pkcs12#26 landed.

@@ -7,7 +7,7 @@ import (
"fmt"
"io/ioutil"

"github.com/Azure/go-pkcs12"
"github.com/Azure/azure-sdk-for-go/Godeps/_workspace/src/golang.org/x/crypto/pkcs12"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is wrong. The import path should be just "golang.org/x/crypto/pkcs12"

Copy link
Member Author

Choose a reason for hiding this comment

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

See https://github.com/Azure/azure-sdk-for-go/blob/master/arm/compute/client.go#L22 for example. This is what godeps does to your import paths. This is not wrong, IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's what happens when you do godep save -r which rewrites your paths IMHO. I don't think we need path rewriting here but let's leave it as is for consistency until somebody complains. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

also keep in mind that if some project imports this package + golang.org/x/crypto/pkcs12 separately, then it'll end up two copies of it, possibly even duplicate types with the same name in the runtime.

@ahmetb
Copy link
Contributor

ahmetb commented Oct 15, 2015

LGTM. Feel free to merge after import path fix.

paulmey added a commit that referenced this pull request Oct 15, 2015
Switch to x/crypto/pkcs12 instead of Azure repo
@paulmey paulmey merged commit 3dcabb6 into Azure:master Oct 15, 2015
@paulmey paulmey deleted the pkcs12 branch December 12, 2015 23:09
richardpark-msft pushed a commit to richardpark-msft/azure-sdk-for-go that referenced this pull request Aug 5, 2021
* Add associated-link-name property to RenewLocks message. Fix connection idle timeout for messages with >=10minsprocessing time.

* Upgrade go-amqp

* Make message.getLinkName package level function

* only set associated link name if available

Co-authored-by: Robert Zakrzewski <Robert.Zakrzewski@tomtom.com>
Co-authored-by: Joel Hendrix <jhendrix@microsoft.com>
benbp pushed a commit to benbp/azure-sdk-for-go that referenced this pull request Sep 15, 2021
* Add associated-link-name property to RenewLocks message. Fix connection idle timeout for messages with >=10minsprocessing time.

* Upgrade go-amqp

* Make message.getLinkName package level function

* only set associated link name if available

Co-authored-by: Robert Zakrzewski <Robert.Zakrzewski@tomtom.com>
Co-authored-by: Joel Hendrix <jhendrix@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants