@dwsupplee Please take a look :)

@dwsupplee Fixed the phpcs errors.

@SurferJeffAtGoogle @bshaffer FYI

@dwsupplee Sorry I broke the tests and now it passes.

This looks pretty good at a glance... quick question, are Metadata values always strings?

Made it an Exception. PTAL

@bshaffer Thanks for the review! PTAL

@dwsupplee
All addressed. PTAL (Please Take Another Look)

Awesome, thanks Takashi!

I came up with a few questions after spending a little time playing around with these changes and familiarizing myself with compute engine metadata. I'm still a bit new and trying to learn the domain, so please forgive me if my questions are naive! :)

Why do we feel this is a good fit for a stream wrapper?

Traditionally I have seen stream wrappers used to decorate a request or to create a simple interface to read only specific bytes from a file. When dealing with potentially large files I could see an interface like this being a perfect fit. I immediately think of the storage api. Are there use cases for only needing to read specific portions of a metadata value?

Would it make sense to create a device to let the user know this functionality is only supported if you are executing the code in a compute instance?

Some of the other APIs in the lib are accessible anywhere and it seems like making the delineation clear would be important.

Do we see this as compliment to or a replacement of a non stream wrapper implementation?

I only see support for the 'attributes' metadata entry - is this the full scope of the wrapper or do we plan on implementing support for the other entries?

Is there a way we could support a user needing to fetch all of their custom keys at once?

@dwsupplee

Why do we feel this is a good fit for a stream wrapper?
Because it is easy to use. It provides only high level user thinking. I want to fetch the metadata value, with a key. The function call below allows users to do it very easily, without knowing the METADATA base URL, and the attributes path.

$value = file_get_contents('gce-metadata://project/mykey');

However, it can be a function like:

$value = ComputeMetadata.get('project/mykey');
// or possibly
$value = ComputeMetadata.get('project/attributes/mykey');
// then we can support metadata other than user-specified

Would it make sense to create a device to let the user know this functionality is only supported if you are executing the code in a compute instance?

What do you mean by device here? Putting some disclaimer will help for sure.

Do we see this as compliment to or a replacement of a non stream wrapper implementation?

I don't know there is any implementation.

I only see support for the 'attributes' metadata entry - is this the full scope of the wrapper or do we plan on implementing support for the other entries?

That's a very good point. See the above example, which can support everything.

Is there a way we could support a user needing to fetch all of their custom keys at once?

I think fetching /project/attributes will give you a list. I'll take a look.

Thanks for the thoughts. I started to think we can just provide a helper function(s). What do you think?

I do like stream wrappers, but I'm a big fan of the something like the second approach for this use case.

I didn't mean anything fancy by device, just some sort of layer that we could put in front of certain APIs to give a user a helpful message to let them know how to proceed using certain functionality. That may be out of the scope of this PR, though :).

I don't know there is any implementation.

You're correct - there isn't yet.

I started to think we can just provide a helper function(s). What do you think?

I really like this approach. Building out some friendly to use helper functions could really help wrap up this functionality well for us.

I am still learning the compute engine api so the hierarchy/naming here may not make perfect sense, but just as simple example what do you think of a client that looks something like this?

use Google\Cloud\Compute\ComputeClient;

$client = new ComputeClient([
    // whatever parameters we need
    'projectId' => 'myMagicalProject'
]);

$client->getCustomMetaData($key, $instance);
// or maybe
$client->metaData()->custom($key, $instance);

@dwsupplee

Ah sorry I intended to make the example static methods, but I made a mistake.

The ComputeClient will have credentials (and related interfaces) for accessing the Compute Engine API. However, the Metadata is available from any Compute Engine instances, so I'd prefer just having helper functions (static methods), rather than having them in the Compute Engine API Client.

How about to have Metadata namespace, and the code looks like:

use Google\Cloud\Compute\Metadata;
$val = Metadata::get('/project/attributes/mykey');

Gotcha, that makes sense!

My only argument against using statics here is that it will make unit testing the app code consuming our client more difficult.

I actually like one of the first statements you made about the stream wrapper,

... without knowing the METADATA base URL, and the attributes path.

I feel like with this approach we are losing part of this by requiring intimate knowledge of the paths in these docs. This approach is flexible, but requires more work for the user. Goal number 1 is a friendly, easy to use API. :)

@dwsupplee

We can also provide some higher level functions like getProjectMetadata, getInstanceMetadata or something like them.

My only argument against using statics here is that it will make unit testing the app code consuming our client more difficult.

That's a very important point. Maybe I should make them instance methods?

use Google\Cloud\Compute\Metadata;
$metadata = new Metadata();
// get project_id (possibly we can provide getProjectId or something because we can cache them)
$val = $metadata->get('/project/project-id');
// get custom metadata (no need for caching layer because they can be changed anytime)
$val = $metadata->getProjectMetadata('mykey');

How does it sound?

👍 very nice.

@dwsupplee
Alright, I think it's ready for another look. PTAL

@dwsupplee
Removed onGCE and relevent bits. PTAL

Looking pretty solid. Thanks for all the hard work Takashi 👍

Alright. I will wait for @bshaffer giving LGTM then I'll merge this.

@bshaffer PTAL

The code and everything looks great! Just a few high-level questions:

1. What happened to the calls to onGCE? I know we were going to refactor to use google/auth, but shouldn't the calls themselves still be somewhere?
2. Should we mention that Compute\Metadata is still available on Managed VMs? It may be confusing to users who don't know Managed VMs actually runs on GCE.

What happened to the calls to onGCE? I know we were going to refactor to use google/auth, but shouldn't the calls themselves still be somewhere?

This library will have dependency on google/auth, so users can just call \Google\Auth\GCECredentials::onGce directly.

Should we mention that Compute\Metadata is still available on Managed VMs? It may be confusing to users who don't know Managed VMs actually runs on GCE.

Alright, will add some comments.

@bshaffer
Done. PTAL

I see, so onGCE was just a helper function 👍

LGTM! 🎉 🎈

Thanks for the review both. Merging

Oh, I don't have a power to merge. @dwsupplee Can you merge this?