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

Issue-543 JdbcDatabaseContainer now accepts Future as image name #582

Closed
wants to merge 5 commits into from

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Feb 12, 2018

Fixes #543

But now I have a question: may be a better approach would be to accept Supplier instead of Future? It seems to suit the desired functionality better.

@iNikem
Copy link
Contributor Author

iNikem commented Feb 12, 2018

@bsideup Any idea why gradle fails on Travis? Works on my machine :)

@AnkBurov
Copy link
Contributor

AnkBurov commented Feb 12, 2018

@iNikem I think you accidentally changed gradle-wrapper.jar and added it to commit. Try revert all changes with gradle-wrapper.jar and commit it.

$ ./gradlew build -x check
Error: Could not find or load main class org.gradle.wrapper.GradleWrapperMain

https://github.com/testcontainers/testcontainers-java/pull/582/files#diff-887141a5ba95530593029fed3eef7684

@bsideup bsideup added this to the next milestone Feb 13, 2018
@bsideup
Copy link
Member

bsideup commented Feb 13, 2018

@kiview

@bsideup
Copy link
Member

bsideup commented Feb 13, 2018

@iNikem we're fixing the issue with corrupted JAR files in the master branch, could you please revert your Gradle Wrapper change and keep only code change for now? :)

@iNikem
Copy link
Contributor Author

iNikem commented Feb 13, 2018

@bsideup I have reverted it to your commit, but it still shows as changed.

@bsideup
Copy link
Member

bsideup commented Feb 13, 2018

@iNikem yes, because of incorrect .gitattributes.
You can do:

git reset master
git add modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
git commit -m "Closes #543 JdbcDatabaseContainer now accepts Future as image name"
git push --force

@iNikem
Copy link
Contributor Author

iNikem commented Feb 13, 2018

@bsideup Ok, Travis is green. Approve and merge? :)

* The '?' character must be included
* @return a full JDBC URL including queryString
*/
protected String constructUrlForConnection(String queryString) {
Copy link
Member

Choose a reason for hiding this comment

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

This change is unrelated, isn't 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.

No, and I haven't done them. Will try to revert...

@bsideup
Copy link
Member

bsideup commented Feb 13, 2018

@iNikem please mention your change in CHANGELOG.md

# Conflicts:
#	modules/jdbc/src/main/java/org/testcontainers/containers/JdbcDatabaseContainer.java
@iNikem
Copy link
Contributor Author

iNikem commented Feb 13, 2018

Now I am quite confused about what is going on with this PR and how changes appear here. Let me decline it and make a fresh new branch for this.

@iNikem iNikem closed this Feb 13, 2018
@iNikem iNikem deleted the issue-543 branch February 13, 2018 15:08
@kiview
Copy link
Member

kiview commented Feb 13, 2018

Yes sorry about the .gitattribute problems, seems the impact was higher than expected 😅

@bsideup bsideup removed this from the next milestone Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants