Skip to content

Conversation

@sergseven
Copy link
Contributor

@sergseven sergseven commented Jan 27, 2022

@sergseven sergseven marked this pull request as draft January 27, 2022 13:55
@Aloren
Copy link
Contributor

Aloren commented Jan 27, 2022

@sergseven
Copy link
Contributor Author

@Aloren
That's a good question.
Actually we are using the approaches mentioned at https://github.com/googleapis/google-cloud-java/blob/main/TESTING.md#on-your-machine-2 for low level(unit or low level integration tests) tests, we also have test projects and test buckets to test our integration with the real world GCS service as well as for E2E tests.

Moreover we have some kind of middle level "component" tests which test both integration and functionality independently of the environment outside of the "component". One of the most important requirement for such a test we have is not to replace any part of a component with test double or test implementation, but test configuration close to production as much as possible on this level. So a component should talk with the dependencies making a network call.
Testcontainers works best for this purposes.
Also, since we have junit & spring boot on the board, the embedded testcontainers work best as well.

@sergseven sergseven marked this pull request as ready for review January 28, 2022 13:38
@sergseven
Copy link
Contributor Author

sergseven commented Feb 9, 2022

Heads up!
Any chance to have it reviewed soon? Any feedback is appreciated.

@Aloren
Copy link
Contributor

Aloren commented Feb 23, 2022

Sorry for the delay.
Is it possible to omit google-cloud-storage from the compile dependencies? Otherwise it might conflict with user's production dependency.
Also I am hesitant to merge the PR since this is not officially supported mock, so as a result end users can face issues that mock is behaving differently than real storage. What is your impression of working with fake google cloud storage? Is it stable enough?

@Aloren
Copy link
Contributor

Aloren commented Feb 23, 2022

Also can you pls rebase to the latest changes.

@sergseven
Copy link
Contributor Author

@Aloren
Sorry for the delay too, was out of my mind with all this hassle...

Is it possible to omit google-cloud-storage from the compile dependencies? Otherwise it might conflict with user's production dependency.

sure, made it as a "provided" dependency.

What is your impression of working with fake google cloud storage? Is it stable enough?

I would say it's pretty much mature, it has about 500+ stars on github and is actively supported and developed by the maintainer.
From my prospective It also has much more features supported than LocalStorageHelper suggested for local testing.

@Aloren Aloren added the feature label Mar 22, 2022
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-storage</artifactId>
<version>${google-cloud-storage.version}</version>
<scope>provided</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add into the documentation that this module consumes/expects this dependency to be provided by the user.


@Slf4j
@Configuration
@ConditionalOnExpression("${embedded.containers.enabled:true}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add
@ConditionalOnClass(com.google.cloud.storage.Storage.class)
to this autoconfiguration, so that is does not startup and fail if user does not have google-cloud-storage on classpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, added

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please move these properties just before dependencies section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, moved

@PostConstruct
protected void init() {
log.info("Creating buckets.");
buckets.forEach(this::createBucket);
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to create bucket via cli, so that we do not have dependency on google-cloud-storage?

Copy link
Contributor

Choose a reason for hiding this comment

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

i've just looked into the transitive dependencies of google-cloud-storage and it looks like it comes with half of the internet :), so if possible please use rest api. for bucket creation, i suppose, this won't be difficult?
https://cloud.google.com/storage/docs/json_api/v1/buckets/insert

/** location for buckets */
private String bucketLocation = "US-CENTRAL1";
/** list with predefined buckets to be created */
private Collection<String> buckets = emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Bucket creation operation supports additional parameters, that may be needed by someone in the future, so lets' replace this instead with:

private Collection<BucketProperties> buckets = emptyList();

static class BucketProperties {
 String name;
// other properties if needed can be added later
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense to me, BucketProperties introduced

* `embedded.google.storage.projectId` `(project id for storage, default is my-project-id)`
* `embedded.google.storage.bucketLocation` `(location for buckets, default is US-CENTRAL1)`

* Predefined buckets:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please rephrase this part to smth like:
embedded.google.storage.buckets creates buckets on startup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The description changed.

GenericContainer<?> container = new GenericContainer<>(ContainerUtils.getDockerImageName(properties))
.withExposedPorts(StorageProperties.PORT)
.withCreateContainerCmdModifier(cmd -> cmd.withEntrypoint(
"/bin/fake-gcs-server",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt we also provide -backend memory option? is is provided in examples and i don't see what is the default value for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default one is filesystem backend, which was used previously.
from now, setting it as a memory backend.
nice catch!

@sergseven
Copy link
Contributor Author

@Aloren
Thank you for a valuable review!

I would discuss the google-cloud-storage dependency topic before actual substitution implementation.

The com.google.cloud.storage.Storage class intended to be the client to GCS is distributed with google-cloud-storage.
What use cases do you see here for a case when a user won't have this dependency already on the class path?

The main use case I would expect is to add embedded-google-storage as a test-scope dependency, so it won't appear on user's prod classpath. The compile scope should be fine then.
Could you please share other possible use cases?

Also noticing that google-cloud-storage is intended to be used along with the bom, so it's pretty unexpected we would mix up client's dependency versions since they should already be managed by a BOM(s).

@Aloren Aloren enabled auto-merge (squash) April 5, 2022 21:03
@Aloren Aloren merged commit 5654108 into PlaytikaOSS:develop Apr 5, 2022
@Aloren
Copy link
Contributor

Aloren commented Apr 7, 2022

@sergseven 2.1.6 was release successfully and is already present in sonatype. Unfortunately Maven central had sync issues on 6th April. I hope that within 48 hours artifacts will be available there as well.
https://status.maven.org/

@sergseven
Copy link
Contributor Author

@sergseven 2.1.6 was release successfully and is already present in sonatype. Unfortunately Maven central had sync issues on 6th April. I hope that within 48 hours artifacts will be available there as well. https://status.maven.org/

Awesome, thank you!
I will try to help to maintain with embedded-gcs support.

@Aloren
Copy link
Contributor

Aloren commented Apr 8, 2022

@sergseven we've released 2.1.7 and it's already available in maven central, please use it. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants