Add support for Script, DeployReport, and DriftReport #106

zijchen · 2022-07-05T23:59:57Z

Addresses #55 and then some

Right now just adds the flow-through from yaml to sqlpackage, nothing fancy, user needs to specify the /OutputPath in the sqlpackage-arguments field.

Depends on changes from #105

TODO: a sample workflow, triggered by PRs, that runs DeployReport and publishes the report as a comment on the PR

github-actions · 2022-07-20T00:20:31Z

This PR is idle because it has been open for 14 days with no activity.

…/deploy-report

chlafreniere · 2022-08-19T18:50:13Z

What's the relationship between main.ts and main.js? I see that main.js is being updated here, but not main.ts. Does main.ts not transpile into lib/main.js?

chlafreniere · 2022-08-19T18:52:41Z

src/AzureSqlAction.ts

@@ -164,13 +164,20 @@ export default class AzureSqlAction {
        let args = '';

        switch (inputs.sqlpackageAction) {
-            case SqlPackageAction.Publish: {
-                args += `/Action:Publish /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}"`;
+            case SqlPackageAction.Publish: 


How about leveraging getSqlpackageActionTypeFromString(), saving the result into a variable, and switching on that variable? Then we wouldn't have to have logic checking for no action being passed in in two different places.

Actually I removed the redundant logic of checking for no action passed in here. SqlPackageAction should be set already at this point coming from the earlier check.

kisantia · 2022-08-19T19:04:44Z

src/AzureSqlAction.ts

                throw new Error(`Not supported SqlPackage action: '${SqlPackageAction[inputs.sqlpackageAction]}'`);
-            }
        }

        if (!!inputs.additionalArguments) {


what happens if the user doesn't pass in the OutputPath or DeployScriptPath for these new actions?

sqlpackage will error out asking for one of those to be specified

zijchen · 2022-08-19T20:58:26Z

What's the relationship between main.ts and main.js? I see that main.js is being updated here, but not main.ts. Does main.ts not transpile into lib/main.js?

@chlafreniere The *.ts file compile into main.js as defined in ./webpack.config.js. So any changes to any of the ts files under src/ will update main.js. main.js is what is used to run the action.

chlafreniere · 2022-08-19T21:44:01Z

What's the relationship between main.ts and main.js? I see that main.js is being updated here, but not main.ts. Does main.ts not transpile into lib/main.js?

@chlafreniere The *.ts file compile into main.js as defined in ./webpack.config.js. So any changes to any of the ts files under src/ will update main.js. main.js is what is used to run the action.

Thanks @zijchen. Not a huge fan of having what are basically build outputs checked in, but I understand that there are some constraints for GH actions in what they require (and also realize that you didn't write this code originally 😄). Certainly not something I'd block on here, just something to think about in the future.

zijchen · 2022-08-19T22:05:32Z

@chlafreniere I totally agree with you that it feels janky to check in a build output. I looked at other actions and it does appear to be a limitation of GH Actions, all of them have a lib/dist folder with a js file in it. Hopefully there is a better way to do this in the future.

chlafreniere · 2022-08-19T22:06:39Z

@chlafreniere I totally agree with you that it feels janky to check in a build output. I looked at other actions and it does appear to be a limitation of GH Actions, all of them have a lib/dist folder with a js file in it. Hopefully there is a better way to do this in the future.

Agreed! Sounds good, thanks for looking into this @zijchen, and thanks for indulging me in my curiosity 😄

chlafreniere · 2022-08-19T22:10:18Z

__tests__/AzureSqlAction.test.ts

+ * @param additionalArguments Additional arguments for this action type.
+ * @returns An ActionInputs objects based on the given action type.
+ */
+function getInputsWithCustomSqlPackageAction(actionType: ActionType, sqlpackageAction: SqlPackageAction, additionalArguments: string = '') {


Can we specify the return type here?

chlafreniere · 2022-08-19T22:15:40Z

__tests__/AzureSqlAction.test.ts

@@ -132,6 +150,29 @@ describe('AzureSqlAction tests', () => {
        expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);  // Verify build succeeds and calls into Publish
        expect(execSpy).toHaveBeenCalledTimes(2);
    });
+
+    describe('validate errors on unsupported sqlpackage action types', () => {


I like how you're testing this negative scenario. How about another test for an invalid sqlpackage action as well? (e.g. Action 'foo')?

This would just ensure that we're handling invalid actions and giving users a consistent error message in future versions of the action

Good point. I moved this test to main.test.ts as the error handling is mainly there. At this point it expects the SqlpackageAction enum type so if it's not one of the supported ones, it becomes undefined. The error handling is further upstream.

…/deploy-report

Benjin · 2022-08-19T23:39:39Z

__tests__/AzureSqlAction.test.ts

-        expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);
-        expect(execSpy).toHaveBeenCalledTimes(1);
-        expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:Publish /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}" /TargetTimeout:20`);
+        it.each(inputs)('Validate %s action with args %s', async (actionName, sqlpackageArgs) => {


Any reason to do it.each() instead of looping with a normal execution?

This is jest's syntax for testing against each row of inputs, kind of similar to NUnit's test cases.

Benjin · 2022-08-19T23:41:57Z

src/AzureSqlAction.ts

@@ -53,15 +52,16 @@ export default class AzureSqlAction {
            await this._executeSqlFile(this._inputs);
        }
        else if (this._inputs.actionType === ActionType.BuildAndPublish) {
-            const dacpacPath = await this._executeBuildProject(this._inputs as IBuildAndPublishInputs);
+            const buildAndPublishInputs = this._inputs as IBuildAndPublishInputs;


Does anything need to be handled in case this doesn't cast successfully?

Any invalid inputs should be handled by this point

* 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]>

zijchen added 3 commits July 5, 2022 14:51

Change inputs

0f459e3

Add other publish like actions

bd4a11b

Add tests

5ce949a

zijchen temporarily deployed to Automation test July 6, 2022 00:00 Inactive

zijchen had a problem deploying to Automation test July 6, 2022 00:00 Failure

zijchen temporarily deployed to Automation test July 6, 2022 00:00 Inactive

zijchen had a problem deploying to Automation test July 6, 2022 00:00 Failure

zijchen temporarily deployed to Automation test July 6, 2022 00:00 Inactive

zijchen changed the base branch from master to v2 July 6, 2022 00:19

github-actions bot added the idle Inactive for 14 days label Jul 20, 2022

zijchen mentioned this pull request Aug 16, 2022

Input changes for v2 #105

Merged

Merge branch 'v2' of https://github.com/Azure/sql-action into zijchen…

3e7a03e

…/deploy-report

zijchen had a problem deploying to Automation test August 18, 2022 23:06 Failure

Fix test

b5d12ba

zijchen temporarily deployed to Automation test August 18, 2022 23:41 Inactive

zijchen marked this pull request as ready for review August 18, 2022 23:42

zijchen removed the idle Inactive for 14 days label Aug 18, 2022

chlafreniere reviewed Aug 19, 2022

View reviewed changes

kisantia reviewed Aug 19, 2022

View reviewed changes

zijchen linked an issue Aug 19, 2022 that may be closed by this pull request

DeployReport support #55

Closed

chlafreniere reviewed Aug 19, 2022

View reviewed changes

PR comments

47c9160

zijchen had a problem deploying to Automation test August 19, 2022 23:36 Failure

Merge branch 'v2' of https://github.com/Azure/sql-action into zijchen…

6cbd202

…/deploy-report

zijchen temporarily deployed to Automation test August 19, 2022 23:40 Inactive

Benjin approved these changes Aug 20, 2022

View reviewed changes

kisantia approved these changes Aug 22, 2022

View reviewed changes

zijchen merged commit 412e851 into v2 Aug 24, 2022

zijchen deleted the zijchen/deploy-report branch August 24, 2022 19:47

zijchen mentioned this pull request Aug 29, 2022

Introducing sql-action v2 #122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Script, DeployReport, and DriftReport #106

Add support for Script, DeployReport, and DriftReport #106

zijchen commented Jul 5, 2022

github-actions bot commented Jul 20, 2022

chlafreniere commented Aug 19, 2022

chlafreniere Aug 19, 2022

zijchen Aug 19, 2022

kisantia Aug 19, 2022

zijchen Aug 19, 2022

zijchen commented Aug 19, 2022

chlafreniere commented Aug 19, 2022

zijchen commented Aug 19, 2022

chlafreniere commented Aug 19, 2022

chlafreniere Aug 19, 2022

zijchen Aug 19, 2022

chlafreniere Aug 19, 2022

zijchen Aug 19, 2022

Benjin Aug 19, 2022

zijchen Aug 22, 2022

Benjin Aug 19, 2022

zijchen Aug 22, 2022

Add support for Script, DeployReport, and DriftReport #106

Add support for Script, DeployReport, and DriftReport #106

Conversation

zijchen commented Jul 5, 2022

github-actions bot commented Jul 20, 2022

chlafreniere commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijchen commented Aug 19, 2022

chlafreniere commented Aug 19, 2022

zijchen commented Aug 19, 2022

chlafreniere commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment