services/galexie: add integration tests for S3 storage. by overcat · Pull Request #5749 · stellar/go-stellar-sdk

overcat · 2025-07-12T09:47:43Z

PR Checklist

PR Structure

This PR has reasonably narrow scope (if not, break it down into smaller PRs).
This PR avoids mixing refactoring changes with feature changes (split into two PRs
otherwise).
This PR's title starts with name of package that is most changed in the PR, ex.
services/friendbot, or all or doc if the changes are broad or impact many
packages.

Thoroughness

This PR adds tests for the most critical parts of the new functionality or fixes.
I've updated any docs (developer docs, .md
files, etc... affected by this change). Take a look in the docs folder for a given service,
like this one.

Release planning

I've reviewed the changes in this PR and if I consider them worthwhile for being mentioned on release notes then I have updated the relevant CHANGELOG.md within the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.
I've decided if this PR requires a new major/minor version according to
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.

What

This is the follow-up PR for #5748. Let's merge #5748 first, and then we'll handle this PR.

Why

See #5748

Known limitations

N/A

urvisavla · 2025-07-16T07:06:48Z

@overcat we've been a bit busy with the Protocol 23 release but we'll review this soon. Thanks!

overcat · 2025-07-16T08:55:33Z

@overcat we've been a bit busy with the Protocol 23 release but we'll review this soon. Thanks!

It's okay, this PR is not urgent.

urvisavla

Great work integrating the S3 tests into the existing framework. The PR looks good.

tamirms · 2025-07-18T16:15:03Z

@overcat this looks great! could you add one more test case to the integration tests? I don't think we have any coverage which exercises the code path where we receive a precondition failed error upon trying to insert a file that already exists. I think the following test case should exercise that path:

initialize empty bucket
scan-and-fill --start 4 --end 10
scan-and-fill --start 8 --end 15

In step (3) galexie should try to insert ledger files 8, 9, and 10. But, it should receive a precondition failed error from AWS and handle that error by skipping over to the next files

overcat · 2025-07-19T03:29:06Z

@overcat this looks great! could you add one more test case to the integration tests? I don't think we have any coverage which exercises the code path where we receive a precondition failed error upon trying to insert a file that already exists. I think the following test case should exercise that path:

initialize empty bucket

scan-and-fill --start 4 --end 10

scan-and-fill --start 8 --end 15

In step (3) galexie should try to insert ledger files 8, 9, and 10. But, it should receive a precondition failed error from AWS and handle that error by skipping over to the next files

Hi @tamirms, I would like the existing TestAppend test to include this scenario. Theoretically, it is best if we can ensure that the object has not changed, but the current datastore interface does not provide this capability. If necessary, we can add a function to the datastore to return the object's identifier.

tamirms · 2025-07-21T15:07:27Z

I would like the existing TestAppend test to include this scenario.

sure, that sounds good!

Theoretically, it is best if we can ensure that the object has not changed, but the current datastore interface does not provide this capability. If necessary, we can add a function to the datastore to return the object's identifier.

I'm not sure what would be the equivalent of that for GCS. Alternatively, you could return the last modified timestamp of the object. I believe that is available for both GCS and S3

urvisavla · 2025-07-21T17:17:38Z

Theoretically, it is best if we can ensure that the object has not changed, but the current datastore interface does not provide this capability. If necessary, we can add a function to the datastore to return the object's identifier.

By this I'm assuming you mean that the object in the datastore is the same as the one we're attempting to upload. We could definitely check for that, but I'm not sure what we’d do with that information beyond just logging it, unless you're suggesting we go ahead and overwrite the object (upload without preconditions) in such a case.

overcat · 2025-07-22T00:44:57Z

Hey @tamirms, I think 6804884 might be what you're looking for. Let me know if I'm on the right track so I can get those unit tests for GetFileLastModified added! cc @urvisavla

tamirms · 2025-07-22T08:42:24Z

Let me know if I'm on the right track so I can get those unit tests for GetFileLastModified added!

@overcat that looks good to me!

overcat · 2025-07-22T10:11:21Z

Thanks @tamirms, I have now added unit tests. 5e32083

BTW. there are many functions in the datastore that use HeadObject; I think we can refactor them, but let's put that in a separate PR.

tamirms · 2025-08-05T19:28:45Z

hi @overcat , I just want to check if you will be updating TestAppend as you mentioned in this comment or if you plan to do that in a separate pr?

Copilot

Pull Request Overview

This PR adds comprehensive integration tests for S3 storage backend support in Galexie, complementing the existing GCS integration tests. The changes enable testing both GCS and S3 datastores to ensure feature parity and proper functionality across different cloud storage providers.

Adds GetFileLastModified method to the DataStore interface and all implementations
Extends integration test suite to support both GCS and S3 storage backends with LocalStack
Updates CI workflows to run tests against both storage types

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
support/datastore/datastore.go	Adds GetFileLastModified method to DataStore interface
support/datastore/s3_datastore.go	Implements GetFileLastModified and refactors to use HeadObject helper
support/datastore/gcs_datastore.go	Implements GetFileLastModified and refactors to use GetFileAttrs helper
support/datastore/mocks.go	Adds mock implementation for GetFileLastModified method
support/datastore/s3_datastore_test.go	Adds lastModified field to mock objects and tests for GetFileLastModified
support/datastore/gcs_test.go	Adds test for GetFileLastModified functionality
services/galexie/internal/integration_test.go	Splits integration tests into GCS and S3 suites with LocalStack support
services/galexie/internal/test/integration_config_template.toml	Adds S3-specific configuration parameters
services/galexie/DEVELOPER_GUIDE.md	Updates documentation to reflect separate test suites
.github/workflows/galexie.yml	Updates CI to run tests for both GCS and S3 storage types
.github/workflows/galexie-release.yml	Updates release workflow to test both storage backends

overcat · 2025-08-06T00:29:43Z

Hi @tamirms, I have already added it in 6804884

https://github.com/stellar/go/blob/6804884f11d9b6a8abc0ed0b3fa7f5c06c63816a/services/galexie/internal/integration_test.go#L134-L137

tamirms · 2025-08-06T08:54:42Z

@overcat sorry, you're correct! I don't know how I missed that code.

Could you also add similar logic to TestScanAndFill() so we can test that scan-and-fill does not overwrite files?

Once that's done I will approve and merge the PR

overcat mentioned this pull request Jul 12, 2025

services/galexie: add implementation for S3 and S3-compatible data storage. #5748

Merged

7 tasks

services/galexie: add integration tests for S3 storage.

0bc05d1

overcat force-pushed the feat-s3-support-dev branch from eb86f15 to 0bc05d1 Compare July 13, 2025 23:34

overcat marked this pull request as ready for review July 13, 2025 23:34

urvisavla requested review from sreuland and tamirms July 17, 2025 19:23

urvisavla approved these changes Jul 18, 2025

View reviewed changes

tamirms reviewed Jul 18, 2025

View reviewed changes

Comment thread .github/workflows/galexie.yml Outdated

update LocalStack image tag in workflows and integration tests

e8aa374

overcat added 2 commits July 22, 2025 08:39

Added GetFileLastModified method to the DataStore interface.

6804884

fix

56521be

fix

1349e86

add unit test cases.

5e32083

Merge branch 'master' into feat-s3-support-dev

34e02cd

Merge branch 'master' into feat-s3-support-dev

231d377

Copilot AI review requested due to automatic review settings August 6, 2025 00:21

Copilot AI reviewed Aug 6, 2025

View reviewed changes

Comment thread services/galexie/internal/integration_test.go

Comment thread services/galexie/internal/integration_test.go

tamirms reviewed Aug 6, 2025

View reviewed changes

Comment thread services/galexie/internal/integration_test.go

overcat and others added 2 commits August 6, 2025 16:55

fix typo

4e87b60

update TestScanAndFill() to scan over existing files

a4d7160

tamirms approved these changes Aug 7, 2025

View reviewed changes

tamirms merged commit 9fbef12 into stellar:master Aug 7, 2025
26 of 29 checks passed

Conversation

overcat commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Structure

Thoroughness

Release planning

What

Why

Known limitations

Uh oh!

urvisavla commented Jul 16, 2025

Uh oh!

overcat commented Jul 16, 2025

Uh oh!

urvisavla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamirms commented Jul 18, 2025

Uh oh!

overcat commented Jul 19, 2025

Uh oh!

tamirms commented Jul 21, 2025

Uh oh!

urvisavla commented Jul 21, 2025

Uh oh!

overcat commented Jul 22, 2025

Uh oh!

tamirms commented Jul 22, 2025

Uh oh!

overcat commented Jul 22, 2025

Uh oh!

tamirms commented Aug 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

overcat commented Aug 6, 2025

Uh oh!

Uh oh!

tamirms commented Aug 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

overcat commented Jul 12, 2025 •

edited

Loading