-
Notifications
You must be signed in to change notification settings - Fork 33
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
Move AWS SPI code into Pekko repo #648
Conversation
pjfanning
commented
May 8, 2024
•
edited
Loading
edited
- See Integrate AWS SPI into pekko-connectors #612
- follow up in get integration tests running in CI for aws-api-pekko-http #655
e0adc7f
to
4a93985
Compare
@mdedetrich I'm thinking of just removing the integration tests. They require real AWS access. There are unit tests that use testcontainers. I think those are enough. Is this ok? |
So we do actually have a real AWS account that is provided by Apache INFRA and it is currently being used by S3 integration tests and there is a github action that triggers it daily off the main branch (note that the reason this is run off the Initially I would suggest trying to enable the tests and if its too much effort we can disable them, file an issue and try and we can try and re-enable them later. The reason I am suggesting this is that the core reason for me pushing for the AWS S3 integration tests is we did have an actual case of a regression that wasn't caught since initially there wasn't tests running against it. |
Oh and I forgot to mention that if you do end up enabling integration tests, you have to work with Apache INFRA so that they can add in the necessary roles/permissions in the AWS account for whatever is being tested. This does require some back and forth so given that it might make sense to do it in a different PR, but regardless I think we should still aim to have these tests in place |
@pjfanning Is this ready? |
I'll leave the integration tests to another time but I have a couple of more things to do with this PR - it is pretty close now. |
import software.amazon.awssdk.services.dynamodb.DynamoDbAsyncClient | ||
import software.amazon.awssdk.services.dynamodb.model._ | ||
|
||
class ITTestDynamoDB |
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.
It seems that IT
and Test
are usually placed at the end
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.
Agree, should be a post fix
I will take a look this weekend |
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.
looks good to me, Can someone else add their review?
This slipped off the table, will check it out tonight |
Would anyone have time to review this PR? I'm hoping to finalise the 1.1.0-M1 RC1 over the next week or so. |
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.
lgtm
Just test naming should be fixed at some point
Co-authored-by: Matthias Lüneberg <[email protected]>
import order scalafmt Update KinesisFirehoseSnippets.java imports
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.
already reviewed. I'm proceeding by approving.
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.
lgtm, sorry for taking a while