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

feat: Metadata Server Detection Configuration #562

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

danielbankhead
Copy link
Contributor

Configure desired metadata server availability check behavior.

(Indirectly) Fixes #548 🦕

d-goog added 2 commits June 22, 2023 16:25
Avoids a type issue with gax that requires a major version bump.
@danielbankhead danielbankhead requested a review from a team as a code owner June 22, 2023 23:31
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jun 22, 2023
`compdoc` wants the babel plugin now
@generated-files-bot
Copy link

Warning: This pull request is touching the following templated files:

@@ -2,6 +2,7 @@
"extends": "./node_modules/gts/tsconfig-google.json",
"compilerOptions": {
"lib": ["es2018", "dom"],
"skipLibCheck": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will create a follow-up issue to revert this in the next major (should be soon).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Background: https://www.typescriptlang.org/tsconfig#skipLibCheck

There's a broken type issue that would require a major version bump to resolve.

Copy link
Contributor

Choose a reason for hiding this comment

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

I too got bitten by this broken type issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking issue: #564

@@ -41,6 +41,7 @@
"json-bigint": "^1.0.0"
},
"devDependencies": {
"@babel/plugin-proposal-private-methods": "^7.18.6",
Copy link
Contributor

@ddelgrosso1 ddelgrosso1 Jun 23, 2023

Choose a reason for hiding this comment

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

I see this was added but is it used someplace or did I misunderstand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, for some reason compdoc wants the babel plugin now, otherwise it’ll throw and fail the docs check (and linkinator would also complain).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: It's a Node 14+ requirement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking issue: #563

@danielbankhead
Copy link
Contributor Author

cc: @TimurSadykov

return false;
case 'bios-only':
return getGCPResidency();
case 'ping-only':

Choose a reason for hiding this comment

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

Maybe we discussed but I forgot.. we don't want to have a default where both ping and bios happen sequentially?

Choose a reason for hiding this comment

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

or default in this case is null and also ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, the default in this case is null and ping - I’m open to changing it.

Copy link

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

LGTM

}

return gcpResidencyCache ? 0 : 3000;
return getGCPResidency() ? 0 : 3000;

Choose a reason for hiding this comment

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

Curious what timeout it is and why exactly 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The timeout is used for availability checking; 0 (indefinite) for GCE and up to 3 secs for other platforms.

@danielbankhead danielbankhead merged commit 8c7c715 into main Jun 28, 2023
@danielbankhead danielbankhead deleted the metadata-detection-override branch June 28, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code running in Cloud Build gets stuck trying to fetch metadata in v5.1.0
4 participants