mobile: using the API listener by default#25899
Conversation
alyssawilk
force-pushed
the
listener
branch
7 times, most recently
from
b6fc545 to
7ff8cb8
Compare
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
alyssawilk
requested review from
Augustyniak,
RyanTheOptimist,
abeyad and
jpsim
as code owners
Signed-off-by: Alyssa Wilk <[email protected]>
Augustyniak
left a comment
Augustyniak
left a comment
There was a problem hiding this comment.
+1 with one nit/typo and one question.
I went over the referenced GH issue to catch up on the decisions that were made with regard to the rollout of the change.
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| cd mobile && ./bazelw test \ | ||
| --build_tests_only \ |
There was a problem hiding this comment.
just curious - was this needed or just an improvement?
There was a problem hiding this comment.
it avoids trying to build mobile/test/common/jni/BUILD on the build we rm -rf the listener file :-P
mobile/test/common/jni/BUILD
Outdated
| native_dep = "libenvoy_jni_with_test_extensions.so", | ||
| ) | ||
|
|
||
| # Same as above but wtih listener extensions too. |
There was a problem hiding this comment.
| # Same as above but wtih listener extensions too. | |
| # Same as above but with listener extensions too. |
Signed-off-by: Alyssa Wilk <[email protected]>
|
Talked with @jpsim. We are going to run the last round of unit/integration tests in Lyft apps to confirm that everything works as expected. We will report back soon. |
|
I think I may not have tested this properly last week because the Swift |
Signed-off-by: Alyssa Wilk <[email protected]>
|
ack - I'd rather wait a few days than land without testing - feel free to mark as waiting on in-house testing if needed! |
|
/wait on Lyft internal testing |
|
Lyft testing on iOS and Android is complete, everything looked good. I'll let Rafal review this PR though. |
There was a problem hiding this comment.
@alyssawilk The change looks good to me. Thank you for verifying @jpsim.
.
|
ugh sorry. happy to roll back and resubmit next week if you'd prefer? |
I think I'm missing something here. As far as I know this hasn't caused any issues? |
It related to my previous (and edited) message where I suggested we wait with the merge until next week. I removed that message after I realized the PR was merged due to auto-merge enabled for the PR (and me approving it). @alyssawilk I think that we are fine here but thank you for offering to help. Apologies for the confusion. I will let you know if we run into any issues and need to revert. |
3 participants
Risk Level: medium
Testing: yes
Docs Changes: no
Release Notes: inline
Fixes envoyproxy/envoy-mobile#2711