Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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!: add spanner rest port and upgrade google-cloud-sdk version #117
base: main
Are you sure you want to change the base?
feat!: add spanner rest port and upgrade google-cloud-sdk version #117
Changes from 2 commits
17ed5dd
53c937c
67590db
0f635ab
73ab5db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
That's definitely a breaking change: publicly visible variable was changed
We can consider a breaking change if we want to update the image version as well.
But if we won't change anything else, doesn't make a lot of sense. We could keep
SPANNER_PORT
as (deprecated) variable= SPANNER_GRPC_PORT
until the next major releaseThere 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.
I can leave
SPANNER_PORT
unchanged (or marked asdeprecated
), but a breaking change is unavoidable since we also need to changepub struct CloudSdkArgs
here. Presumably, this is less likely to break people, but it's still a breaking API change.Do we care about that other breaking change?
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.
Well, in that case I think we can go only with breaking change flag.
Thus, I think it's ok to bump the version as well. But better to preserve the registry as dockerhub
I'd suggest to refactor
CloudSdkArgs
a bit to avoid such breaking changes in the future, e.g: make the fields private and expose builder-like functions. So we can add new arguments in the future without breaking changes, just by adding.with_my_arg(...)
(just for example)WDYT?
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.
Yeah, that makes sense. I'll take a stab at this later today.
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.
Hi there!
Any progress? We're going to prepare a new release with breaking changes soon.
So would be nice to incorporate this one as well
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.
I took a stab at this, but it was a larger endeavour than I expected. After digging into the details, I realized that the current abstractions are more or less "in the way".
The reality is that the
gcloud
command line is structured like so:where:
global args
applies to all emulators (because it actually applies to thegcloud
command)emulator
is one ofspanner | bigtable | ...
, but may requiregcloud beta emulators
or not (depends on the version of the image)emulator args
are specific to the emulator, but also happen to overlap (e.g.:--host-port host:port
is present on all of them, but it's not like it's guaranteed to be the case)The current structure "mixes" things, for example the
project
field onDatastore
is actually part ofglobal args
and should be available on all emulators, not just this one.Lastly, after comparing with other similar containers, like cockroachdb,
elastic-search
andconsul
, they all just take arbitrary lists of arguments without trying to formalize them into types.So I was faced with 2 options:
Vec<String>
s, one forgcloud
and one for the emulatorOption 2 seems better IMO because it will be more amenable to using different versions of the sdk (which may have different command line arguments then the ones formalized in this library). So it would ultimately be less work to maintain, but makes it harder for users to use...
Both options are actually a non-trivial amount of work (2 is probably less, but still). Maybe an approach would be to make this a new submodule? e.g.:
gcloud_sdk
which would map to thecloud-sdk
image, but not assume you want to run emulators. There are actually other reasons why you may want to use this image as-is without using the emulators, for example, you can use that image to do stuff against "actual" GCP, like create test instances of databases, or GCS buckets etc.Any opinions?
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.
In general, we could go with 2
Vec<String>
, but accept something like&[impl Into<String>]
(but provide methods with meaningful names). This way, users can define their own structures which implementDisplay
(e.g enums).For test scenarios it's usually ok to hardcode some values or create simple wrappers.
Otherwise it can be too complicated to support all possible arguments. It could lead to a mess (especially across different versions)
For example, ENV variables also could be represented as typed structs, but it's too complicated for testing purposes. And additional complexity for maintainers.
IMO, providing strong-typed interface makes sense only for some required and limited parameters.
Emulator
looks like a good candidate, even though it's not really requiredIn general this sounds reasonable, but it's not entirely accurate because these reasons don't look like the case we want to support as is:
These cases just don't seem like the "test containers" use case.