-
Notifications
You must be signed in to change notification settings - Fork 53
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(gcp): current region methods #399
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the idea here! See suggestions...
/// thrown. | ||
Future<String> regionFromMetadataServer() async { | ||
const host = 'http://metadata.google.internal/'; | ||
final url = Uri.parse('$host/computeMetadata/v1/instance/region'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should DRY up this logic. Maybe add a metadata_server.dart
file and share the logic with gcp_project.dart
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prefanatic thoughts on my thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevmoo Sorry about the delay - I took a few stabs at drying up this implementation, but didn't like the direction I found myself in.
One attempt at buttoning this up included defining MetadataServerClient
(name withstanding) which exposed these region and project endpoints. However, I was unhappy with aspects related to error handling; we disclose some nice resolution regarding which environment variables to provide in leu of the metadata server. It felt weird to me to move that into such a metadata_server.dart
.
In an attempt with leaving the error handling out, I ended up with an implementation that's slightly analogous to http.read, where computeRegion
and computeProject
would open up a client, try and return the metadata result, or catch into statement about environment variables needed. This slimmed up the original methods a little bit, but now introduced some arguably duplicative withClient
semantics to retain backwards compatibility in those methods.
I think this approach is worth taking? Let me know if I'm on the right track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of this? #421
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looks much nicer. I wish I thought of using enhanced enums!
I left some small documentation nits over there for the generalization, in the review.
@prefanatic – it seems like Do you just want some utilities to access metadata? Do you have a case where you want to overload the function region locally? I don't want to over design this! |
Typically my use case for running locally, not on GCP, revolves around ensuring my environment is set up appropriately with the environment variables these methods would be first looking for. I then just assume the metadata access "just works" when it's inside GCP. In terms of "utilities to access metadata" and over designing in general - I suppose the answer lies in how easily mapped metadata endpoints are to this enhanced enum approach? I don't have sufficient context over what this service offers. We'd also assume in this case that many, or all, metadata endpoints have associated environment variables. |
This PR implements methods to retrieve the region in which the current cloud instance is running. Structurally, these methods are analogous to projectId supporting methods, found within
gcp/lib/src/gcp_project.dart
Usage of these new methods follow a similar pattern:
Practically, these new methods are copy-pastes, substituting
projectId
withregion
. In my original proposal within #392, I perhaps erroneously utilized location rather than region. After combing through some GCP documentation, it looks like region is perhaps the more widely utilized term?I am looking for additional feedback on the code documentation for these methods. Most of the documentation from
gcp_project.dart
was brought forward, with some subjective edits to represent instance & region vs. project Ids.Regarding the metadata server, I linked to instance metadata to provide further documentation on what these instance endpoints provide, however, I was unable to find
v1/instance/region
within this public documentation. Is this particular endpoint not documented there for a reason?I tried combing through search to find various environment variables that currently exist, that could represent the instance's region. I was only successful in finding two discretely populated variables. If there are more, I would like to add them in to add some redundancy in the same way the projectId is evaluated.
Finally, in regards to unit tests, I brought forward the same pattern used to test
computeProjectId
. This includes the TODO comment regarding stubs for the metadata server under test.Closes #392