Skip to content

Client Certificate auth for Confidential Ledger#28645

Merged
christothes merged 6 commits intomainfrom
chriss/CL-certs
Jun 22, 2022
Merged

Client Certificate auth for Confidential Ledger#28645
christothes merged 6 commits intomainfrom
chriss/CL-certs

Conversation

@christothes
Copy link
Member

@christothes christothes commented May 10, 2022

@ghost ghost added the Azure.Core label May 10, 2022
@azure-sdk
Copy link
Collaborator

API change check for Azure.Security.ConfidentialLedger

API changes have been detected in Azure.Security.ConfidentialLedger. You can review API changes here

API changes

+         public ConfidentialLedgerClient(Uri ledgerUri, CertificateCredential certificateCredential, ConfidentialLedgerClientOptions options = null);

@azure-sdk
Copy link
Collaborator

API change check for Azure.Core

API changes have been detected in Azure.Core. You can review API changes here

API changes

+     public class CertificateCredential {
+         public CertificateCredential(X509Certificate2 certificate);
+         public X509Certificate2 ClientCertificate { get; }
+     }
+         public IList<CertificateCredential> ClientCertificates { get; }

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

  1. You can't make local changes to eng/common. They need to be in Azure/azure-sdk-tools in the same directory.
  2. Suggestion: I wouldn't rename (move) the .psm1. It's not needed with import-module -path, which we could also document if necessary.

@christothes
Copy link
Member Author

  1. You can't make local changes to eng/common. They need to be in Azure/azure-sdk-tools in the same directory.

Doh! I forgot about that.

  1. Suggestion: I wouldn't rename (move) the .psm1. It's not needed with import-module -path, which we could also document if necessary.

I had some issues with -path not working consistently. I'm fine either way, but this seems to be more reliable.

@heaths
Copy link
Member

heaths commented May 10, 2022

I had some issues with -path not working consistently. I'm fine either way, but this seems to be more reliable.

I'm okay with that change then. The only reason for the block was local changes to eng/common. FWIW, I do like to test in a local repo like -net, but shouldn't check in like that.

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Security.ConfidentialLedger
Azure.Core

@check-enforcer
Copy link

This pull request is protected by Check Enforcer.
For more information about how to run a pipeline against this pull request, see this.

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

I don't see anything blocking, but had a couple of nits and an open question (again, not blocking).

@heaths
Copy link
Member

heaths commented Jun 21, 2022

Given the merge commit - though GitHub somehow showed a "condensed version" (wonder when they added that nifty little feature) - I'd generally recommend rebasing + force push. Most often GitHub still shows reviewers only subsequent commits.

If one were to look at the whole merge commit:

image

I see that often in the specs repo and have them do a rebase instead.

@christothes christothes merged commit 4f54c9c into main Jun 22, 2022
@christothes christothes deleted the chriss/CL-certs branch June 22, 2022 22:37
zhihaoxue pushed a commit to zhihaoxue/azure-sdk-for-net that referenced this pull request Jul 27, 2022
sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-net that referenced this pull request Dec 7, 2022
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.

4 participants