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

Fix for Sybase ASE 15.7 identification string #1221

Merged
merged 3 commits into from Mar 21, 2016
Merged

Fix for Sybase ASE 15.7 identification string #1221

merged 3 commits into from Mar 21, 2016

Conversation

hakkika
Copy link
Contributor

@hakkika hakkika commented Mar 1, 2016

No description provided.

@@ -131,7 +131,7 @@ public static DbSupport createDbSupport(Connection connection, boolean printInfo
}

//Sybase ASE support
if (databaseProductName.startsWith("ASE")) {
if (databaseProductName.startsWith("Adaptive Server Enterprise")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should really accept both otherwise you'll break the latest version

Copy link
Contributor

Choose a reason for hiding this comment

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

@super132 Thoughts? (This is related to #1220)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there actually a SAP ASE database that returns "ASE" or is it the same as Sybase ASE that returns "Adaptive Server Enterprise"?

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I tested in ASE 15.2, it returns ASE. I think the name is different in 15.7?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then let's make sure we allow both for Flyway 4.0.1

Copy link
Contributor

Choose a reason for hiding this comment

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

@hakkika Can you update this PR to allow both the old and the new name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fix should now allow both names. Can you see the change in the pull
request?

On Tue, Mar 1, 2016 at 4:38 PM, Axel Fontaine notifications@github.com
wrote:

In
flyway-core/src/main/java/org/flywaydb/core/internal/dbsupport/DbSupportFactory.java
#1221 (comment):

@@ -131,7 +131,7 @@ public static DbSupport createDbSupport(Connection connection, boolean printInfo
}

    //Sybase ASE support
  •    if (databaseProductName.startsWith("ASE")) {
    
  •    if (databaseProductName.startsWith("Adaptive Server Enterprise")) {
    

@hakkika https://github.com/hakkika Can you update this PR to allow
both the old and the new name?


Reply to this email directly or view it on GitHub
https://github.com/flyway/flyway/pull/1221/files#r54574564.

t. Kari Häkkinen

@axelfontaine
Copy link
Contributor

Have you run the Sybase tests to see if they all pass with your version?

@axelfontaine axelfontaine added this to the Flyway 4.0.1 milestone Mar 1, 2016
@hakkika
Copy link
Contributor Author

hakkika commented Mar 1, 2016

I have not run any Sybase tests because I am not able to build flyway unless skipping all tests.

@axelfontaine
Copy link
Contributor

Do they pass in your IDE?

@hakkika
Copy link
Contributor Author

hakkika commented Mar 1, 2016

I am sorry, but I am not a Java developer and running the tests in IDE is not something I know how to do either. I just am interested in Sybase ASE support in flyway.

axelfontaine pushed a commit that referenced this pull request Mar 21, 2016
Fix for Sybase ASE 15.7 identification string
@axelfontaine axelfontaine merged commit 2aa2896 into flyway:master Mar 21, 2016
@axelfontaine
Copy link
Contributor

Thanks! Merged.

axelfontaine pushed a commit to flyway/flywaydb.org that referenced this pull request Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants