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(gcp): current region methods #399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

prefanatic
Copy link

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:

final computedRegion = await computeRegion();
print(computedRegion);
...
// Outputs: us-central1

final environmentRegion = regionFromEnvironment();
print(environmentRegion);
...
// Outputs: us-central1

Practically, these new methods are copy-pastes, substituting projectId with region. 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

Copy link
Collaborator

@kevmoo kevmoo left a 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');
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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

Copy link
Author

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.

@kevmoo
Copy link
Collaborator

kevmoo commented Jul 18, 2023

@prefanatic – it seems like CLOUDSDK_COMPUTE_REGION and FUNCTION_REGION

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!

@prefanatic
Copy link
Author

@prefanatic – it seems like CLOUDSDK_COMPUTE_REGION and FUNCTION_REGION

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide access to an instance's region from metadata
2 participants