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: use metadata package #95

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

Conversation

JulienBreux
Copy link
Member

Description

Use of the metadata package to show users how to properly obtain the metadata of an instance in Cloud Run.

Note

Removal of the region part in favor of the zone part.
The region part is no longer part of the metadata package and API.

Signed-off-by: Julien Breux <[email protected]>
@JulienBreux
Copy link
Member Author

cc @steren @grayside

@steren
Copy link
Collaborator

steren commented Jul 12, 2024

The original idea of this demo container was "no dependencies", and we preferred 20 lines to call metadata server that importing a package for that.

That being said, we then added some packages for cloudevents, so I'm fine with this change.

@steren steren self-assigned this Jul 12, 2024
Copy link
Collaborator

@steren steren left a comment

Choose a reason for hiding this comment

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

Zone is different from Region.

As you can see in the original code, we parse the zone to extract the region.

Also, the html template expects a region and talks about region

Copy link
Contributor

@grayside grayside left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it's great to simplify to make better use of the available libraries.

@@ -157,7 +115,7 @@ func main() {
Service: service,
Revision: revision,
Project: project,
Region: region,
Zone: zone,
Copy link
Contributor

Choose a reason for hiding this comment

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

blocker: Cloud Run uses region not zone. While we can extract the region from the zone, we can't use it directly. We should teach using region and how to extract region from zone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks for this relevant comment!
I think region is missing from metadata package.
I need to check (sorry for delay) here: https://github.com/googleapis/google-cloud-go/blob/main/compute/metadata/metadata.go

@steren
Copy link
Collaborator

steren commented Jul 12, 2024

Also note that Cloud Run metadata server now returns the region directly (``/computeMetadata/v1/instance/region`, but I am not totally sure this is supported on GCE.

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.

3 participants