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

doc(AIP-4114): Add documentation for ECP and new env var default. #1451

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andyrzhao
Copy link
Contributor

No description provided.

@andyrzhao andyrzhao requested a review from a team as a code owner November 25, 2024 18:35
@andyrzhao andyrzhao requested review from shinfan, westarle and noahdietz and removed request for shinfan and westarle December 4, 2024 20:25
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

Hey hey, happy to review this but someone from the auth team needs to review it as well: https://github.com/orgs/aip-dev/teams/auth

aip/auth/4114.md Outdated
1. Delegates signing operations to keystores, so private keys never leave the security realm.

Please see [ECP Public Documentation][2] for details on ECP configuration. ECP is
fully supported in Google Golang libraries and gCloud SDK as of 2023.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we want to mention which languages/surfaces support it here and not in say the doc site where there are already examples and better means of expressing that? Otherwise we will need to keep this updated whenever a new language/surface supports it.

That said, I'm not opposed to including it if no where else is reasonable - it's good to know and not have to ask around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point - I will omit the last part since the AIP is a recommendation and not a status tracker. Ideally there should be a separate public documentation today on which feature is supported in various clients by language, but no such thing exists AFAIK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep I agree and TBH the client library AIPs don't have this level of tracking though we regularly ask each other which language supports which feature.

aip/auth/4114.md Show resolved Hide resolved
@andyrzhao
Copy link
Contributor Author

andyrzhao commented Dec 4, 2024

Hey hey, happy to review this but someone from the auth team needs to review it as well: https://github.com/orgs/aip-dev/teams/auth

Thanks for the review Noah! Due to several rounds of re-orgs and team shuffling, the auth owners list will need a refresher, I will try to update the list to correctly reflect the new auth owners.

@noahdietz
Copy link
Collaborator

When you're ready for me to review again, hit the re-request button :)
image

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.

2 participants