Skip to content

ci/mobile: register listener#24490

Merged
Augustyniak merged 7 commits intomainfrom
test-fixing-objc-hello-world-app
Dec 12, 2022
Merged

ci/mobile: register listener#24490
Augustyniak merged 7 commits intomainfrom
test-fixing-objc-hello-world-app

Conversation

@Augustyniak
Copy link
Copy Markdown
Contributor

@Augustyniak Augustyniak commented Dec 12, 2022 •
edited
Loading

Commit Message:
Additional Description: Register default listener. Envoy Mobile crashes in optimized builds without this change. The crash was introduced in #24294. The objc hello world app CI job was disabled in #24363 to make main green again.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Dec 12, 2022

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24490 was opened by Augustyniak.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Dec 12, 2022

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @jpsim

🐱

Caused by: #24490 was opened by Augustyniak.

see: more, trace.

@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Dec 12, 2022

this is registering the regular listener not the API listener
Is it Ok to remove this once the API listener lands? The goal of that whole series was to remove the 2M

@Augustyniak
Copy link
Copy Markdown
Contributor Author

Augustyniak commented Dec 12, 2022

this is registering the regular listener not the API listener

trying this based off the error message that we saw:

[2022-12-09 17:20:23.372][13984307][critical][assert] [library/common/engine.cc:97] panic: Didn't find a registered implementation for name: 'envoy.listener_manager_impl.default'

@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Dec 12, 2022

Is it Ok to remove this once the API listener lands? The goal of that whole series was to remove the 2M

Yes it's ok to remove this once the lightweight listener manager lands.

jpsim
jpsim previously approved these changes Dec 12, 2022
View reviewed changes
@Augustyniak Augustyniak marked this pull request as ready for review December 12, 2022 17:54
@Augustyniak
register listener
076a0c8 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Revert "ci: disable running the Objective-C integration app (#24478)"
fc342d8 
This reverts commit 0a8043c.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
fixes
433fdb3 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
revert a part of a PR
85aa658 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Revert "Revert "ci: disable running the Objective-C integration app (#…
201132f 
…24478)""

This reverts commit 8a01a75ff6888fa120a97087fd0d5309dca170ae.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Revert "revert a part of a PR"
7321517 
This reverts commit 6472ac439c4072480f0c03a5890bee3e7d1d84cd.

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
lint fix
9d4340c 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak Augustyniak force-pushed the test-fixing-objc-hello-world-app branch from 95a0777 to 9d4340c Compare December 12, 2022 17:56
@Augustyniak Augustyniak enabled auto-merge (squash) December 12, 2022 19:22
@Augustyniak Augustyniak changed the title ci/mobile: register api listener ci/mobile: register listener Dec 12, 2022
@Augustyniak Augustyniak merged commit 8d9f80d into main Dec 12, 2022
@Augustyniak Augustyniak mentioned this pull request Dec 12, 2022
Merged
Augustyniak added a commit that referenced this pull request Dec 13, 2022
@Augustyniak
ci: revert disable running the Objective-C integration app (#24478)" (#…
5438479 
…24496)

Commit Message:
Additional Description: Re-enables CI job as it should be green again as #24490 and after registering quick socket related protos.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]
@mattklein123 mattklein123 deleted the test-fixing-objc-hello-world-app branch January 20, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist Awaiting requested review from RyanTheOptimist RyanTheOptimist is a code owner

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@abeyad abeyad Awaiting requested review from abeyad abeyad is a code owner

1 more reviewer

@jpsim jpsim jpsim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jpsim jpsim

Labels

mobile

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Augustyniak @alyssawilk @jpsim