Skip to content

chore: Run tests in parallel, wait before read (S3)#19320

Merged
kodiakhq[bot] merged 8 commits intomainfrom
fix/update-sdk-manually-s3
Oct 7, 2024
Merged

chore: Run tests in parallel, wait before read (S3)#19320
kodiakhq[bot] merged 8 commits intomainfrom
fix/update-sdk-manually-s3

Conversation

@disq
Copy link
Copy Markdown
Member

@disq disq commented Oct 3, 2024

Getting the S3 plugin ready for the StreamingBatchWriter update. Incorporates cloudquery/filetypes#579 and cloudquery/plugin-sdk#1921

@disq disq added the no automerge Block automatic merging label Oct 3, 2024
@disq disq requested review from a team and savme and removed request for a team October 4, 2024 14:50
@disq disq changed the title fix: Update SDK chore: Run tests in parallel, wait before read (S3) Oct 4, 2024
@disq disq added automerge Automatically merge once required checks pass and removed no automerge Block automatic merging labels Oct 4, 2024
t.Fatal(fmt.Errorf("failed to close client: %w", err))
}

time.Sleep(2 * time.Second)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we signal completion instead of waiting an arbitrary amount of time?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But it has already been completed, code runs fine without the sleep if you're lucky (if S3 behaves well). This is to "unflake" the test. We could also mock S3 here, but that would probably go outside of the scope of this PR, which is to get the StreamingBatchWriter updates safely over the line.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it! Totally makes sense to keep mocking out of scope of this PR.

Perhaps, something like assert.Eventually could be a better fit here? Feel free to disregard the suggestion and proceed with the merge, though

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good tip, implemented in all three PRs. Can't do it in the file PR unfortunately, without changing the test concept.

@savme savme removed the automerge Automatically merge once required checks pass label Oct 4, 2024
@disq disq added the automerge Automatically merge once required checks pass label Oct 7, 2024
@kodiakhq kodiakhq bot merged commit cef27a9 into main Oct 7, 2024
@kodiakhq kodiakhq bot deleted the fix/update-sdk-manually-s3 branch October 7, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/plugin/destination/s3 automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants