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

Retry connection with user DB if master connection fails #104

Merged
merged 8 commits into from
Jul 12, 2022
Merged

Conversation

zijchen
Copy link
Contributor

@zijchen zijchen commented Jun 29, 2022

Fixes #89

@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen temporarily deployed to Automation test June 29, 2022 01:48 Inactive
@zijchen zijchen marked this pull request as ready for review June 29, 2022 17:42
@zijchen zijchen requested review from dzsquared and yorek as code owners June 29, 2022 17:42
src/SqlUtils.ts Outdated Show resolved Hide resolved
src/SqlUtils.ts Outdated
* @param useMaster If true, uses "master" instead of the database specified in @param config. Every other config remains the same.
* @param suppressError If true, will not throw connection errors. Non-connection errors will always be thrown.
* @returns If connection succeeds, returns empty string. If connection fails due to firewall rule, returns the client's IP address.
* Otherwise returns undefined if there were errors but were suppressed by @param suppressError.

Choose a reason for hiding this comment

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

it's a little confusing that the return type has 3 different states that the caller needs to know: ipAddress = connection failed due to firewall rule, empty string = successfully connected, and undefined = error when connecting.

Can this return an object that has properties that make it more clear what happened? Maybe something like

{
    success: boolean,
    ipAddress?: string
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a ConnectionResult interface

@zijchen zijchen temporarily deployed to Automation test July 1, 2022 23:17 Inactive
@zijchen zijchen temporarily deployed to Automation test July 1, 2022 23:17 Inactive
@zijchen zijchen temporarily deployed to Automation test July 1, 2022 23:17 Inactive
@zijchen zijchen temporarily deployed to Automation test July 1, 2022 23:17 Inactive
@zijchen zijchen temporarily deployed to Automation test July 1, 2022 23:17 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen zijchen temporarily deployed to Automation test July 5, 2022 22:12 Inactive
@zijchen
Copy link
Contributor Author

zijchen commented Jul 6, 2022

@mabster could you give this a try to see if it addresses your issue?

You can use the action directly from this PR branch by changing your yaml to

- uses: azure/sql-action@zijchen/issue89

@mabster
Copy link

mabster commented Jul 6, 2022

@zijchen Sure thing! Are the parameters the same? So, like this?

 - name: Apply EF migration script
        uses: azure/sql-action@zijchen/issue89
        with:
          connection-string: ${{ secrets.CONNECTION_STRING }}
          sql-file: ${{env.DOTNET_ROOT}}/myapp/migrate.sql

@zijchen
Copy link
Contributor Author

zijchen commented Jul 6, 2022

@mabster Yes the inputs are the same as of the latest commit.

@mabster
Copy link

mabster commented Jul 6, 2022

OK, this is odd.

I generate my migrate.sql script using dotnet ef migrations script, and then run it using your new version, and I just get a series of errors as my output:

Error: Incorrect syntax near ''.
Error: Incorrect syntax near 'GO'.
Error: Incorrect syntax near 'GO'.
Error: Incorrect syntax near 'GO'.
Error: Incorrect syntax near 'GO'.
Error: Incorrect syntax near 'GO'.
Error: Incorrect syntax near 'GO'.
... lots more of the same ...
Error: Failed to execute query.

Is there a "verbose" input I can pass to get more detail on the error?

The script is pretty straight forward. Here's the first few statements from one I generated locally:

IF OBJECT_ID(N'[__EFMigrationsHistory]') IS NULL
BEGIN
    CREATE TABLE [__EFMigrationsHistory] (
        [MigrationId] nvarchar(150) NOT NULL,
        [ProductVersion] nvarchar(32) NOT NULL,
        CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId])
    );
END;
GO

BEGIN TRANSACTION;
GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20220330033849_InitialCreate')
BEGIN
    CREATE TABLE [Courses] (
        [Id] int NOT NULL IDENTITY,
        [Code] nvarchar(max) NOT NULL,
        [Name] nvarchar(max) NOT NULL,
        [IsEnabled] bit NOT NULL,
        CONSTRAINT [PK_Courses] PRIMARY KEY ([Id])
    );
END;
GO

UPDATE: Oops, I think I was using the wrong connection string. Running the workflow again now to see if I get different results.

UPDATE 2: Nope, same result with the right connection string. What can I try to get more detail in the errors?

@mabster
Copy link

mabster commented Jul 6, 2022

The file generated by dotnet ef migrations script is "UTF-8 with BOM" encoded. Could that be an issue? Is that 100% compatible with plain-old "UTF-8"?

scriptContents = fs.readFileSync(inputs.sqlFile, "utf8");

@zijchen
Copy link
Contributor Author

zijchen commented Jul 6, 2022

@mabster This could be because this branch is based off the v2 branch, which uses node-mssql instead of sqlcmd. GO is more of a sqlcmd statement than a native T-SQL statement. Could you remove the GO statements in your script and try again?

@zijchen
Copy link
Contributor Author

zijchen commented Jul 6, 2022

I can see this being an issue for folks migrating to v2, created #107 for it.

@mabster
Copy link

mabster commented Jul 6, 2022

Could you remove the GO statements in your script and try again?

Honestly not sure how I do that in a GitHub Actions workflow. Is there some sort of file replace text step I can use?

@mabster
Copy link

mabster commented Jul 8, 2022

Oh snap I got it to work.

Added this step between "generate the script" and "apply the script":

 - name: Remove 'GO' from migration script
        run: (Get-Content ${{env.DOTNET_ROOT}}/migrate.sql | ? { $_ -ne 'GO' }) | Set-Content ${{env.DOTNET_ROOT}}/migrate.sql

It's a bit of a hack but the script ran fine once the "GO" lines were removed.

@zijchen
Copy link
Contributor Author

zijchen commented Jul 12, 2022

Thanks for the confirmation. I'll merge this PR now, but we are considering integrating go-sqlcmd for the script action, which would restore some of the sqlcmd functionality.

@zijchen zijchen merged commit 39541c4 into v2 Jul 12, 2022
@zijchen zijchen deleted the zijchen/issue89 branch July 12, 2022 20:03
zijchen added a commit that referenced this pull request Sep 28, 2022
* v2 - Use tedious mssql module instead of sqlcmd (#96)

* Use tedious mssql library instead of sqlcmd

* Fix mssql connection

* Fix SqlUtils tests

* Use config instead of connection string

* Replace conn string builder with mssql config

* Connect to master db

* Restore connection string validation regex

* PR comments, fix error handling

* Update main.js

* Use try catch for error handling

* Fix typo

* v2 - Change script action from sqlcmd to mssql query (#99)

* Change script action from sqlcmd to mssql query

* Update action.yml

* Fully qualify Table1 in sql script

* Add more debug logging

* Clone config before changing db to master

* Cleanup

* Set TEST_DB name before cleanup

* Use runner.temp

* Always cleanup

* PR comments

* Fix database cleanup step in pr-check (#101)

* Debug script contents

* Fix sed command

* Remove debug

* v2 - Add support for AAD password, AAD default, and AAD service principal auth (#100)

* Use tedious mssql library instead of sqlcmd

* Fix mssql connection

* Fix SqlUtils tests

* Use config instead of connection string

* Replace conn string builder with mssql config

* Connect to master db

* Restore connection string validation regex

* AAD auth

* Add support for client and tenant IDs

* Add more debug messaging

* Fix connection string find array

* Make client-id and tenant-id action inputs

* Fix error handling

* More fixes

* Use try catch instead

* Add tests to pr-check.yml

* Change script action from sqlcmd to mssql query

* Update action.yml

* Fully qualify Table1 in sql script

* Add more debug logging

* Clone config before changing db to master

* Cleanup

* Set TEST_DB name before cleanup

* Use runner.temp

* Always cleanup

* Add tests for different auth types

* Mask tenant and client IDs

* Add AAD password test to pr-check.yml

* Fix yaml

* Limit max-parallel to 2

* Add test for service principal

* PR comments

* Fix typo

* Cleanup sqlcmd references (#102)

* Retry connection with user DB if master connection fails (#104)

* Retry with DB connection if master fails

* Add tests

* Add ConnectionResult interface

* Add missing doc comment

* PR comments

* PR Comments

* Download and extract go-sqlcmd before main action (#108)

* Add setup script to download go-sqlcmd

* Add sqlcmd call to pr-check.yml

* Add bz2 specific extract tar

* Move setup code to main

* Move setup code to main

* Fix casing of Setup.ts

* Use go-sqlcmd for script action (#113)

* call go-sqlcmd for script action

* Fix auth options not flowing through

* Add test cases

* Restore sqlcmd variable in cleanup script

* Fix pr-check cleanup

* Undo changes to pr-check.yml

* Undo pr-check.yml changes

* PR comments

* Change inputs (#105)

* Update SQL API version to 2021-11-01 (#117)

* Add support for Script, DeployReport, and DriftReport (#106)

* Change inputs

* Add other publish like actions

* Add tests

* Fix test

* PR comments

* Add mockReturnValue for auth type test

* Remove MacOS from ci.yml

* links to documentation on each argument type (#123)

* v2 - Remove client-id and tenant-id as inputs (#124)

* Set tenantId and clientId only when passed in

* Default to empty string

* Default to empty string

* Replace mssql call with go-sqlcmd query

* Capture sqlcmd error message

* Add more debug

* Capture stdout too

* Fix config passing

* Completely remove client-id and tenant-id

* cleanup

* Update sqlcmd query

* adding connection string format to docs (#138)

adding connection string format to docs

Co-authored-by: Drew Skwiers-Koballa <[email protected]>
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.

3 participants