Skip to content

binder: Promote out of experimental status#9669

Merged
ejona86 merged 13 commits into
grpc:masterfrom
cbianchi-7:remove-experimental-for-app-interaction-sdk
Nov 30, 2022
Merged

binder: Promote out of experimental status#9669
ejona86 merged 13 commits into
grpc:masterfrom
cbianchi-7:remove-experimental-for-app-interaction-sdk

Conversation

@cbianchi-7

@cbianchi-7 cbianchi-7 commented Nov 2, 2022

Copy link
Copy Markdown
Contributor

Removing the @experimentalapi annotation for APIs used by the App Interaction Library to enable gRPC to be used as dependency for IPCs for Jetpack.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Nov 2, 2022 •
edited
Loading

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

markb74
markb74 reviewed Nov 3, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java
markb74
markb74 reviewed Nov 3, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
@cbianchi-7
Updating the public APIs on SecurityPolicies with experimental annota…
a0fc978 
…tion and leaving the internalOnly() without.
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 3, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 3, 2022
ejona86
ejona86 reviewed Nov 3, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
@ejona86 ejona86 mentioned this pull request Nov 3, 2022
Open
ejona86
ejona86 reviewed Nov 3, 2022
View reviewed changes

@ejona86 ejona86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These comments are from our API review meeting. Overall, mostly nits. I don't expect most of these to be addressed directly in this PR. The two things that are impactful that we noticed involve IBinderReceiver and non-final AndroidComponentAddress. We can accept not touching IBinderReceiver. But final-izing AndroidComponentAddress seems reasonably important.

Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java
Comment thread binder/src/main/java/io/grpc/binder/IBinderReceiver.java
Comment thread binder/src/main/java/io/grpc/binder/IBinderReceiver.java
Comment thread binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
Comment thread binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
Comment thread binder/src/main/java/io/grpc/binder/AndroidComponentAddress.java Outdated
Comment thread binder/src/main/java/io/grpc/binder/AndroidComponentAddress.java Outdated
@ejona86

ejona86 commented Nov 4, 2022

Copy link
Copy Markdown
Member

If the user adds interceptors, they will run before the security interceptor, because interceptors wrap services. The last one is closest to the network.

grpc-java/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java

Line 111 in a82ea0c

BinderTransportSecurity.installAuthInterceptor(this);

markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java Outdated
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java Outdated
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java Outdated
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java Outdated
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java Outdated
markb74
markb74 reviewed Nov 9, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/AndroidComponentAddress.java
ejona86
ejona86 reviewed Nov 15, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 16, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 16, 2022
@cbianchi-7

cbianchi-7 commented Nov 29, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Have addressed and resolved all existing comments / changes. Are there any additional thoughts from folks?
@ejona86 , @jdcormie , @markb74

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 29, 2022
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 29, 2022
ejona86
ejona86 reviewed Nov 29, 2022
View reviewed changes

@ejona86 ejona86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I noticed one thing. I think I'll fix it up myself and then approve.

Comment thread binder/src/main/java/io/grpc/binder/BinderInternal.java
@cbianchi-7

cbianchi-7 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

That sounds good to me. Thanks for running the additional checks as well. All looks good.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 29, 2022
ejona86
ejona86 approved these changes Nov 29, 2022
View reviewed changes

@ejona86 ejona86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll let markb74 merge.

@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 29, 2022
@markb74 markb74 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 30, 2022
jdcormie
jdcormie reviewed Nov 30, 2022
View reviewed changes

@jdcormie jdcormie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if readers in this forum will understand the connection to "app interaction sdk" in the PR description. How about "Promote binder out of experimental status" or similar?

@cbianchi-7 cbianchi-7 changed the title Remove experimental for app interaction sdk Promoting binder out of experimental status Nov 30, 2022
@cbianchi-7

cbianchi-7 commented Nov 30, 2022

Copy link
Copy Markdown
Contributor Author

@jdcormie , I think that's a good suggestion to give better change context. I updated the description accordingly using the active tense.

@ejona86 ejona86 merged commit d6aa0ea into grpc:master Nov 30, 2022
@ejona86 ejona86 changed the title Promoting binder out of experimental status binder: Promote out of experimental status Nov 30, 2022
jdcormie
jdcormie reviewed Nov 30, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/AndroidComponentAddress.java

/**
* Returns the Authority which is the package name of the target app.
* See {@link android.content.ComponentName}.

@jdcormie jdcormie Nov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

between paragraphs (https://google.github.io/styleguide/javaguide.html#s7.1.2-javadoc-paragraphs)

jdcormie
jdcormie reviewed Nov 30, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/BinderInternal.java
public class BinderInternal {

/**
* Set the receiver's {@link IBinder} using {@link IBinderReceiver#set(IBinder)}.

@jdcormie jdcormie Nov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Sets" (https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment)

jdcormie
jdcormie reviewed Nov 30, 2022
View reviewed changes
Comment thread binder/src/main/java/io/grpc/binder/SecurityPolicies.java
}

/**
* Creates a default {@link SecurityPolicy} that checks authorization based on UID.

@jdcormie jdcormie Nov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"... that allows access only to callers with the same UID as the current process."

@cbianchi-7 cbianchi-7 mentioned this pull request Dec 1, 2022
Merged
YifeiZhuang added a commit to YifeiZhuang/grpc-java that referenced this pull request Dec 12, 2022
@YifeiZhuang
Revert "binder: Promote out of experimental status (grpc#9669)"
662c5a9 
This reverts commit d6aa0ea.
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jdcormie jdcormie jdcormie left review comments

@ejona86 ejona86 ejona86 approved these changes

+1 more reviewer

@markb74 markb74 markb74 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cbianchi-7 @ejona86 @jdcormie @markb74 @grpc-kokoro