Skip to content

Remove getInfoHostString and use connectable hostnames. #5079

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Oct 7, 2022
Merged

Remove getInfoHostString and use connectable hostnames. #5079

merged 9 commits into from
Oct 7, 2022

Conversation

yuchenshi
Copy link
Member

Description

This is a refactor with a subtle behavior change to use correctly-formatted URLs with hostname fixes. This marks the start of a quest to fix IPv6 and DNS related problems.

When given a 0.0.0.0 hostname, we'll try to connect over 127.0.0.1 instead to make sure max client compatibility. EmulatorRegistry.getInfo now returns the transformed hostname by default, since this is usually the desirable behavior. A new helper method is added for creating an API client to an emulator -- a common enough pattern that worth standardizing. This PR also uses EmulatorRegistry.url for most URL constructions and has removed getInfoHostString, which causes more confusions than it helps.

Scenarios Tested

Unit test added for the new connectableHostname helper.

Basic manual testing like starting the emulators.

I'll rely on our CI to catch any remaining errors.

Sample Commands

N/A

@yuchenshi yuchenshi force-pushed the ys/refactor-emulator-info branch 2 times, most recently from 1e11fb4 to dcd8c87 Compare October 6, 2022 21:20
bkendall
bkendall requested changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

I think we'll want to manually test some webframeworks stuff here for sanity.

@yuchenshi yuchenshi force-pushed the ys/refactor-emulator-info branch from 496ab9a to fa56b2f Compare October 6, 2022 22:49
@yuchenshi yuchenshi requested a review from bkendall October 6, 2022 23:18
christhompsongoogle
christhompsongoogle reviewed Oct 7, 2022
View reviewed changes
src/emulator/controller.ts
const emulators: EmulatorInfo[] = [];
if (experiments.isEnabled("webframeworks")) {
for (const e of EMULATORS_SUPPORTED_BY_UI) {
// TODO: Double check if this actually works -- we're early in the startup
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add name/ldap

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know the right contact yet but I've started a thread for it internally. If I cannot identify a contact, then I'll just create an internal bug for this TODO.

christhompsongoogle
christhompsongoogle reviewed Oct 7, 2022
View reviewed changes
src/emulator/functionsEmulator.ts Outdated
setEnvVarsForEmulators(envs);
const eventarcEmulator = this.getEmulatorInfo(Emulators.EVENTARC);
if (eventarcEmulator) {
// TODO: Why do we set the protocol here, but no protocol during emulators:exec?
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add name/ldap

@christhompsongoogle
Copy link
Contributor

christhompsongoogle commented Oct 7, 2022 •
edited
Loading

Question: What's the semantic difference between "isRunning" and "getInfo" when checking if an Emulator is running/active/enabled - is info populated before isRunning? Why switch from one to the other?

@yuchenshi
Copy link
Member Author

Question: What's the semantic difference between "isRunning" and "getInfo" when checking if an Emulator is running/active/enabled - is info populated before isRunning? Why switch from one to the other?

It is more or less equivalent, and there's no actual difference in timing. But isRunning is preferred because it captures the intention better.

Deeper reason: When using getInfo, if later one needs to construct a URL, they will naturally get the host and port from the info and try simple string interpolation, which works most of the time but fails in the edge cases in PR description. It is much safer to just do a boolean test and use EmulatorRegistry.url or EmulatorRegistry.client later if needed.

Future consideration: I'm about to create follow-up PRs to change the way emulator listens on hosts (with DNS in mind) and it is necessary to audit the situations where the host is absolutely needed. Less getInfo means less places to track down and potentially modify later.

Side benefit: isRunning does less work than getInfo -- a bit more performance, less moving pieces.

@yuchenshi yuchenshi force-pushed the ys/refactor-emulator-info branch from 82c9224 to 531baf1 Compare October 7, 2022 03:35
@codecov-commenter
Copy link

Codecov Report

Base: 55.75% // Head: 55.95% // Increases project coverage by +0.20% 🎉

Coverage data is based on head (855eeb9) compared to base (9f86b0f).
Patch coverage: 38.28% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5079      +/-   ##
==========================================
+ Coverage   55.75%   55.95%   +0.20%     
==========================================
  Files         305      306       +1     
  Lines       20529    20469      -60     
  Branches     4145     4132      -13     
==========================================
+ Hits        11445    11454       +9     
+ Misses       8101     8039      -62     
+ Partials      983      976       -7
Impacted Files Coverage Δ
src/emulator/auth/index.ts 22.38% <ø> (ø)
src/emulator/auth/utils.ts 87.23% <0.00%> (ø)
src/emulator/controller.ts 12.80% <ø> (ø)
src/emulator/eventarcEmulator.ts 9.19% <0.00%> (-0.81%) ⬇️
src/emulator/extensionsEmulator.ts 30.43% <0.00%> (ø)
src/emulator/firestoreEmulator.ts 22.00% <0.00%> (-0.23%) ⬇️
src/emulator/hub.ts 18.62% <0.00%> (+0.18%) ⬆️
src/emulator/hubExport.ts 14.43% <0.00%> (+0.14%) ⬆️
src/emulator/pubsubEmulator.ts 11.84% <0.00%> (-1.15%) ⬇️
src/emulator/storage/apis/gcloud.ts 5.40% <0.00%> (+0.07%) ⬆️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yuchenshi yuchenshi enabled auto-merge (squash) October 7, 2022 04:53
@yuchenshi yuchenshi force-pushed the ys/refactor-emulator-info branch from 855eeb9 to ef03ec7 Compare October 7, 2022 15:49
@yuchenshi yuchenshi merged commit f71ac92 into master Oct 7, 2022
@joehan joehan mentioned this pull request Nov 15, 2022
Merged
@taeold taeold mentioned this pull request Dec 13, 2022
Merged
taeold added a commit that referenced this pull request Dec 15, 2022
@taeold
Fix bug where environment variables for an emulated function did not …
0c3a688 
…consider emulators running remotely. (#5269)

#5079 refactored the way to set environment variables on Functions Emulator. As implemented, the refactored function does not consider "remote" emulators i.e. emulator started by another instance of `emulators:start` command. This broke users who relies this to connect `functions:shell` command to the remote emulators.

Fixes #5225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkendall bkendall bkendall approved these changes

@christhompsongoogle christhompsongoogle christhompsongoogle approved these changes

Assignees

@bkendall bkendall

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yuchenshi @christhompsongoogle @codecov-commenter @bkendall