proposal icon indicating copy to clipboard operation
proposal copied to clipboard

Published 20 hours ago • verified publisher icon grpc

A20: Create ALTS credential API for C core and C++

Open yihuazhang opened this issue 6 years ago • 8 comments

Discussion at: https://groups.google.com/forum/#!topic/grpc-io/CI0xJgcPdIs

This change is Reviewable

yihuazhang avatar Mar 29 '18 20:03 yihuazhang

I think we're on L28 now.

This was L26: https://github.com/grpc/proposal/pull/63

Please update the PR title accordingly as well.

dfawley avatar Mar 29 '18 23:03 dfawley

Sorry, another nit about the PR: please add the affected language name(s) in the title of the gRFC and the PR. Thanks!

dfawley avatar Mar 30 '18 15:03 dfawley

As per our discussion, there's no need for this gRFC if the C++ APIs are only experimental at this point. It's certainly a good idea for users to comment on this gRFC and for you to incorporate that feedback into the APIs, but we don't need to actually merge this until you're ready to make the APIs non-experimental (at which point we would not be able to change them without breaking people).

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

L28-c++-grpc-api-extensions-alts.md, line 33 at r1 (raw file):

## Proposal

The first part of proposal adds the following C APIs of creating ALTS client

The C-core API is not considered public and does not need to be included in the gRFC.

Comments from Reviewable

markdroth avatar Apr 05 '18 14:04 markdroth

Thanks Mark for the review. C-core APIs are removed from the gRFC proposal.

yihuazhang avatar Apr 05 '18 16:04 yihuazhang

It looks like I was wrong about that -- we do actually require a gRFC for core API changes, as per:

https://github.com/grpc/proposal/blob/master/P3-grfcs-for-core-api-changes.md

Sorry for steering you wrong about that.

markdroth avatar Sep 05 '18 19:09 markdroth

@markdroth

Np. I will prepare for two gRFC's for the changes made to gRPC core related to ALTS (#14727) and local credentials(#15909).

yihuazhang avatar Sep 05 '18 20:09 yihuazhang

@markdroth, instead of creating a new gRFC for ALTS C core API changes, how about re-using the current gRFC by adding in those changes? Since ALTS is still at the experimental phase, I don't think the new gRFC (if we create one) should get merged as well, in which case it makes more sense to keep a single gRFC that contains both changes (i.e., C core and C++ API changes).

I will prepare for a PR that will add comments to the current ALTS C core surface API saying the API is for experimental.

yihuazhang avatar Sep 06 '18 23:09 yihuazhang

Yes, it's fine to include both the core and C++ API changes in this single gRFC.

markdroth avatar Sep 10 '18 19:09 markdroth