Skip to content
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

Set fix localstack container version and set keep-alive header manually / explicit #31

Merged
merged 2 commits into from
Sep 28, 2019

Conversation

wem
Copy link
Contributor

@wem wem commented Sep 17, 2019

No description provided.

@wem wem force-pushed the fix/local_stack_version_keep_alive branch from 5e4d0b4 to b1a301d Compare September 17, 2019 18:18
@vietj vietj requested a review from aesteve September 19, 2019 12:11
@aesteve
Copy link
Collaborator

aesteve commented Sep 19, 2019

Tests are working locally (but on my laptop they always did).
I'm wondering if this fix would fix the tests on travis, though.

@wem
Copy link
Contributor Author

wem commented Sep 20, 2019

The problem will happen when you pull the currently latest Localstack image (0.10.3). If you locally already has the 0.10.2 and you don't pull, it will work

@aesteve
Copy link
Collaborator

aesteve commented Sep 20, 2019

Thanks a lot, may I ask you something, please?

Would you mind changing: https://github.com/reactiverse/aws-sdk/blob/master/.travis.yml#L16

to:

      script: ./gradlew --no-daemon -i -Dtests.integration=localstack --stacktrace jacocoTestReport

in your branch?

So that we can see if Travis build now passes?

We'll merge your work anyway, and thanks a lot btw. But it'd be really awesome if your changes fix the Travis build!

@aesteve aesteve self-assigned this Sep 20, 2019
@wem
Copy link
Contributor Author

wem commented Sep 20, 2019

Done, but still fail. I dont now Travis, is the runner a new instance? It looks like there is maybe already a Localstack container instance running from a previous test run? A fun fact is as well, that the busy port is the one of CloudFormation which is useless in the tests.

Another question. Are you interested in Kotlin extensions for this library in a separate sub-lib like aws-sdk-lang-kotlin? I think about a collection of AWS SDK extensions with the Await approach, like most of the other Vert.x libraries have.
I ask because it would be some work :)

@aesteve
Copy link
Collaborator

aesteve commented Sep 20, 2019

Ok, I can see on Travis it's still failing unfortunately :(
At least we gave it a try. Thank you!

@aesteve aesteve closed this Sep 20, 2019
@aesteve aesteve reopened this Sep 20, 2019
@aesteve aesteve self-requested a review September 20, 2019 10:09
.travis.yml Outdated
@@ -13,7 +13,7 @@ jobs:
- stage: test
name: Build on OpenJDK 8
jdk: openjdk8
script: ./gradlew --no-daemon -i --stacktrace jacocoTestReport
script: ./gradlew --no-daemon -i -Dtests.integration=localstack --stacktrace jacocoTestReport
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
script: ./gradlew --no-daemon -i -Dtests.integration=localstack --stacktrace jacocoTestReport
script: ./gradlew --no-daemon -i --stacktrace jacocoTestReport

Back to the "non-integration" tests

@aesteve
Copy link
Collaborator

aesteve commented Sep 20, 2019

Regarding your other question, I have to think.
I like working in Kotlin but not sure I have enough knowledge to maintain such a set of extension methods in the long run. Especially with coroutines since I'm not using them that much (mostly using Kotlin + RxJava).

@aesteve aesteve added the bug Something isn't working label Sep 20, 2019
@aesteve
Copy link
Collaborator

aesteve commented Sep 26, 2019

@wem could you please revert the change I requested above, please.

So that we can merge your work :) (I don't have permission to "apply suggestion", unfortunately).

Thank you.

@wem
Copy link
Contributor Author

wem commented Sep 26, 2019

Hi @aesteve ,

i will do it this evening. Sorry for delay.

@wem wem force-pushed the fix/local_stack_version_keep_alive branch from 528032f to b1a301d Compare September 28, 2019 06:34
@wem
Copy link
Contributor Author

wem commented Sep 28, 2019

Done

@aesteve
Copy link
Collaborator

aesteve commented Sep 28, 2019

Thanks a lot.

I'll just take your question out of this PR to open an issue, so that we don't forget about it.
I think Reactiverse maintainers should give their opinion :)

Thanks a lot for your contribution.

@aesteve aesteve merged commit 2dc6ded into reactiverse:master Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants