🎊

This is awesome news. I am a bit concerned about changes to the "public" interface. I am going to discuss with my colleagues, I suspect we will need to change some things in a backwards compatible way.

+kokoro:run

Reviewed 69 of 69 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @vnvo2409)

google/cloud/storage/client_options.h, line 63 at r1 (raw file):

 * By default, several environment variables are read to configure the client:
 *
 * - `CLOUD_STORAGE_EMULATOR_ENDPOINT`: if set, use this http endpoint to

I think this would make the change a breaking change, and I am not sure we need to do it. I mean, the naming will be a bit awkward without this change, but OTOH, I am not sure we need to break any build scripts / tests that application developers may have created.

To be clear, I am not sure this is a blocker, but I am not sure it is not either. Let me chat with some folks here, including @devjgm .

google/cloud/storage/doc/storage-main.dox, line 61 at r1 (raw file):

- `CLOUD_STORAGE_ENABLE_TRACING=raw-client,http` enables all logging.

- `CLOUD_STORAGE_EMULATOR_ENDPOINT=...` override the default endpoint used by

Putting a pin here to think about backwards compatibility.

google/cloud/storage/emulator/emulator.py, line 680 at r1 (raw file):

                    None,
                )
                blob.metadata.metadata["x_emulator_transfer_encoding"] = ":".join(

This is also something that application developers may be using in their own testing.

google/cloud/storage/emulator/README.md, line 45 at r1 (raw file):

### return-broken-stream

Set request headers with `x-goog-emulator-instructions: return-broken-stream`.

I know folks in other libraries (notably golang, maybe Python and Java too) use these headers to do their own testing. I think we should not break their code. What if we supported both x-goo-emulator-instructions and x-goog-testbench-instructions with the same semantics?

google/cloud/storage/tools/run_testbench_utils.sh, line 19 at r1 (raw file):

source module lib/io.sh

EMULATOR_PID=0

I note this file is not renamed, any reason why it should not be?

To be clear, I like the emulator name better too. Just that in some places we may need to support both the old name and the new name.

+kokoro:run

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @vnvo2409)

I think we need to preserve backwards compatibility where we can. See below for details.

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @vnvo2409)

google/cloud/storage/client_options.h, line 63 at r1 (raw file):

This is definitely a breaking change, I think the best way to proceed is to continue to support the old option, but change the documentation to recommend the new one.

These changes are Okay, but I will be asking for other changes elsewhere.

google/cloud/storage/client_options.cc, line 34 at r2 (raw file):

namespace internal {

I think we want a helper function like so:

absl::optional<std::string> GetEmulator() {
  auto emulator = GetEnv("CLOUD_STORAGE_EMULATOR_ENDPOINT");
  if (emulator) return emulator;
  return GetEnv("CLOUD_STORAGE_TESTBENCH_ENDPOINT");
}

this function would be used in this file like so:

std::string JsonEndpoint(ClientOptions const& options) {
  return GetEmulator().value_or(options.endpoint_) + "/storage/" + options.version();
}

This would preserve backwards compatibility and we would signal to users (in the documentation) that the new name is preferred.

google/cloud/storage/well_known_headers_test.cc, line 28 at r2 (raw file):

/// @test Verify that CustomHeader works as expected.
TEST(WellKnownHeader, CustomHeader) {
  CustomHeader header("x-goog-emulator-instructions", "do-stuff");

FYI, these changes are Okay, but the emulator needs to support the old values for backwards compatibility, more on that later.

google/cloud/storage/doc/storage-main.dox, line 61 at r1 (raw file):

I think we should add a description for the old option, something like:

- `CLOUD_STORAGE_TESTBENCH_ENDPOINT=...` **DEPRECATED** use `CLOUD_STORAGE_EMULATOR_ENDPOINT` instead.

google/cloud/storage/emulator/emulator.py, line 680 at r1 (raw file):

I think we need to preserve the backwards compatible behavior. please keep the old names for these attributes, e.g. x_testbench_transfer_encoding.

google/cloud/storage/emulator/grpc_server.py, line 139 at r2 (raw file):

            request, bucket, context
        )
        upload.metadata.metadata["x_emulator_upload"] = "resumable"

Ditto, please preserve the returned name. If we must, we can return both the new and old names.

google/cloud/storage/emulator/README.md, line 41 at r2 (raw file):

## Force Failures

You can force the following failures by using the `x-goog-emulator-instructions` header.

Maybe add something like:

... using the `x-goog-emulator-instructions` header. The `x-goog-testbench-instructions` header is deprecated, but supported for backwards compatibility, please change your code to use `x-goog-emulator-instructions` instead.

google/cloud/storage/emulator/gcs/object.py, line 93 at r2 (raw file):

    ):
        if context is None:
            instruction = request.headers.get("x-goog-emulator-instructions")

maybe:

instruction = request.headers.get("x-goog-emulator-instructions") or request.headers.get("x-goog-testbench-instructions")

But if there is a more pythonic way to say "use a if present, b if not present" then I am all for it.

google/cloud/storage/emulator/gcs/object.py, line 175 at r2 (raw file):

            "bucket": bucket.name,
            "name": object_name,
            "metadata": {"x_emulator_upload": "simple"},

As above, we need to preserve backwards compatibility for these.

google/cloud/storage/examples/storage_examples_common.cc, line 27 at r2 (raw file):

bool UsingEmulator() {
  return !google::cloud::internal::GetEnv("CLOUD_STORAGE_EMULATOR_ENDPOINT")

I think we should support the backwards compatibility option here. Thoughts?

google/cloud/storage/internal/retry_object_read_source.cc, line 74 at r2 (raw file):

    return result;
  }
  bool has_emulator_instructions = false;

FYI, I thought about asking for backwards compatibility here, but it is not needed, this is our own code to support our own tests, we are fine.

google/cloud/storage/tests/object_basic_crud_integration_test.cc, line 155 at r2 (raw file):

StatusOr<Client> CreateNonDefaultClient() {
  auto emulator =
      google::cloud::internal::GetEnv("CLOUD_STORAGE_EMULATOR_ENDPOINT");

FYI, another case where backwards compatibility does not seem necessary.

+kokoro:run

With this change a few x_testbench_* attributes in the object metadata are becoming x_emulator_*. That is still a breaking change. I would be Okay if we created both, with the old ones mark as deprecated (maybe to be removed in 90 days or something). Thoughts?

Reviewed 12 of 12 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewable status: complete! all files reviewed, all discussions resolved