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

mobile: consistently compiling out admin code #24364

Merged
merged 5 commits into from
Feb 8, 2023

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Dec 5, 2022

Changing the default E-M build to not include the admin interface. It is enabled for swift CI due to regression tests.

Risk Level: low
Testing: CI
Docs Changes: n/a
Release Notes: n/a

@repokitteh-read-only
Copy link

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: #24364 was opened by alyssawilk.

see: more, trace.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #24364 was opened by alyssawilk.

see: more, trace.

@alyssawilk
Copy link
Contributor Author

@jpsim @Augustyniak if I fix up the C++ tests to work sans admin, what do you folks think about landing this, vs switching only some of the CI bots to not include admin?

@jpsim
Copy link
Contributor

jpsim commented Dec 5, 2022

if I fix up the C++ tests to work sans admin, what do you folks think about landing this, vs switching only some of the CI bots to not include admin?

I think we agreed at a community meeting a while back that an end-goal should be to remove admin entirely, exposing programmatic interfaces to access stats instead whenever it's useful.

So I'm definitely onboard with finding a way to never need admin to be enabled.

@Augustyniak
Copy link
Contributor

Fine with landing this.

I think we agreed at a community meeting a while back that an end-goal should be to remove admin entirely, exposing programmatic interfaces to access stats instead whenever it's useful.

During the aforementioned community call I mentioned that I use admin endpoint for the purpose of local dev (when working with stats specifically). While I think that admin should be disabled by default I would be more for keeping an option to enable it as needed if possible at least until we figure out programatic interfaces for accessing stats.

@alyssawilk alyssawilk added the no stalebot Disables stalebot from closing an issue label Dec 15, 2022
@alyssawilk alyssawilk force-pushed the admin branch 3 times, most recently from e2729be to 2eb0c84 Compare January 3, 2023 21:53
@alyssawilk
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24364 (comment) was created by @alyssawilk.

see: more, trace.

@alyssawilk alyssawilk force-pushed the admin branch 2 times, most recently from 6a90640 to 0f92460 Compare January 4, 2023 14:58
@alyssawilk
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24364 (comment) was created by @alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk marked this pull request as ready for review February 1, 2023 14:15
RyanTheOptimist
RyanTheOptimist previously approved these changes Feb 1, 2023
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Nice!

abeyad
abeyad previously approved these changes Feb 1, 2023
@abeyad
Copy link
Contributor

abeyad commented Feb 1, 2023

Probably want to change the title of the PR to remove "WIP"

@alyssawilk alyssawilk changed the title WIP: consistent admin mobile: consistently compiling out admin code Feb 1, 2023
jpsim
jpsim previously approved these changes Feb 1, 2023
@alyssawilk alyssawilk enabled auto-merge (squash) February 2, 2023 13:43
@abeyad
Copy link
Contributor

abeyad commented Feb 3, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #24364 (comment) was created by @abeyad.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk dismissed stale reviews from jpsim, abeyad, and RyanTheOptimist via b446fbc February 7, 2023 21:11
@alyssawilk alyssawilk merged commit b7a5f87 into envoyproxy:main Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mobile no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants