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.

Sign up for GitHub

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

Add support for Script, DeployReport, and DriftReport #106

Merged
merged 7 commits into from
Aug 24, 2022
Merged

Add support for Script, DeployReport, and DriftReport #106

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0f459e3
Change inputs
zijchen Jul 5, 2022
bd4a11b
Add other publish like actions
zijchen Jul 5, 2022
5ce949a
Add tests
zijchen Jul 5, 2022
3e7a03e
Merge branch 'v2' of https://github.com/Azure/sql-action into zijchen…
zijchen Aug 18, 2022
b5d12ba
Fix test
zijchen Aug 18, 2022
47c9160
PR comments
zijchen Aug 19, 2022
6cbd202
Merge branch 'v2' of https://github.com/Azure/sql-action into zijchen…
zijchen Aug 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ The definition of this GitHub Action is in [action.yml](https://github.com/Azure
path:

# optional when using a .sql script, required otherwise
# sqlpackage action on the .dacpac or .sqlproj file, only Publish is supported now
# sqlpackage action on the .dacpac or .sqlproj file, supported options are: Publish, Script, DeployReport, DriftReport
action:

# optional app (client) ID when using Azure Active Directory authentication
Expand Down
1 change: 1 addition & 0 deletions __testdata__/TestProject/sql-action.sqlproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@
<Name>sql-action</Name>
<DSP>Microsoft.Data.Tools.Schema.Sql.SqlAzureV12DatabaseSchemaProvider</DSP>
<ModelCollation>1033, CI</ModelCollation>
<ProjectGuid>{829ec500-6bb6-42c3-a1ab-ab621099d153}</ProjectGuid>
</PropertyGroup>
</Project>
143 changes: 113 additions & 30 deletions __tests__/AzureSqlAction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,31 @@ jest.mock('fs');
describe('AzureSqlAction tests', () => {
afterEach(() => {
jest.restoreAllMocks();
})

it('executes dacpac action for DacpacAction type', async () => {
let inputs = getInputs(ActionType.DacpacAction) as IDacpacActionInputs;
let action = new AzureSqlAction(inputs);

let getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');
let execSpy = jest.spyOn(exec, 'exec').mockResolvedValue(0);
});

await action.execute();
describe('validate sqlpackage calls for DacpacAction', () => {
const inputs = [
['Publish', '/TargetTimeout:20'],
['Script', '/DeployScriptPath:script.sql'],
['DriftReport', '/OutputPath:report.xml'],
['DeployReport', '/OutputPath:report.xml']
];

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) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let inputs = getInputsWithCustomSqlPackageAction(ActionType.DacpacAction, SqlPackageAction[actionName], sqlpackageArgs) as IDacpacActionInputs;
let action = new AzureSqlAction(inputs);

let getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');
let execSpy = jest.spyOn(exec, 'exec').mockResolvedValue(0);

await action.execute();

expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);
expect(execSpy).toHaveBeenCalledTimes(1);
expect(execSpy).toHaveBeenCalledWith(`"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}" ${sqlpackageArgs}`);
});
});

it('throws if SqlPackage.exe fails to publish dacpac', async () => {
let inputs = getInputs(ActionType.DacpacAction) as IDacpacActionInputs;
let action = new AzureSqlAction(inputs);
Expand Down Expand Up @@ -81,24 +90,33 @@ describe('AzureSqlAction tests', () => {
expect(await action.execute().catch(() => null)).rejects;
});

it('should build and publish database project', async () => {
const inputs = getInputs(ActionType.BuildAndPublish) as IBuildAndPublishInputs;
const action = new AzureSqlAction(inputs);
const expectedDacpac = path.join('./bin/Debug', 'TestProject.dacpac');

const parseCommandArgumentsSpy = jest.spyOn(DotnetUtils, 'parseCommandArguments').mockResolvedValue({});
const findArgumentSpy = jest.spyOn(DotnetUtils, 'findArgument').mockResolvedValue(undefined);
const getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');
const execSpy = jest.spyOn(exec, 'exec').mockResolvedValue(0);
describe('validate build actions', () => {
const inputs = [
['Publish', '/p:DropPermissionsNotInSource=true'],
['Script', '/DeployScriptPath:script.sql'],
['DriftReport', '/OutputPath:report.xml'],
['DeployReport', '/OutputPath:report.xml']
];

await action.execute();
it.each(inputs)('Validate build and %s action with args %s', async (actionName, sqlpackageArgs) => {
const inputs = getInputsWithCustomSqlPackageAction(ActionType.BuildAndPublish, SqlPackageAction[actionName], sqlpackageArgs) as IBuildAndPublishInputs;
const action = new AzureSqlAction(inputs);
const expectedDacpac = path.join('./bin/Debug', 'TestProject.dacpac');

expect(parseCommandArgumentsSpy).toHaveBeenCalledTimes(1);
expect(findArgumentSpy).toHaveBeenCalledTimes(2);
expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);
expect(execSpy).toHaveBeenCalledTimes(2);
expect(execSpy).toHaveBeenNthCalledWith(1, `dotnet build "./TestProject.sqlproj" -p:NetCoreBuild=true --verbose --test "test value"`);
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:Publish /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${expectedDacpac}"`);
const parseCommandArgumentsSpy = jest.spyOn(DotnetUtils, 'parseCommandArguments').mockResolvedValue({});
const findArgumentSpy = jest.spyOn(DotnetUtils, 'findArgument').mockResolvedValue(undefined);
const getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');
const execSpy = jest.spyOn(exec, 'exec').mockResolvedValue(0);

await action.execute();

expect(parseCommandArgumentsSpy).toHaveBeenCalledTimes(1);
expect(findArgumentSpy).toHaveBeenCalledTimes(2);
expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);
expect(execSpy).toHaveBeenCalledTimes(2);
expect(execSpy).toHaveBeenNthCalledWith(1, `dotnet build "./TestProject.sqlproj" -p:NetCoreBuild=true --verbose --test "test value"`);
expect(execSpy).toHaveBeenNthCalledWith(2, `"SqlPackage.exe" /Action:${actionName} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${expectedDacpac}" ${sqlpackageArgs}`);
});
});

it('throws if dotnet fails to build sqlproj', async () => {
Expand Down Expand Up @@ -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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const inputs = [ ['Extract'], ['Export'], ['Import'] ];

it.each(inputs)('Throws for unsupported action %s', async (actionName) => {
const inputs = getInputsWithCustomSqlPackageAction(ActionType.DacpacAction, SqlPackageAction[actionName]);
const action = new AzureSqlAction(inputs);

const getSqlPackagePathSpy = jest.spyOn(AzureSqlActionHelper, 'getSqlPackagePath').mockResolvedValue('SqlPackage.exe');

let error: Error | undefined;
try {
await action.execute();
}
catch (e) {
error = e;
}

expect(error).toBeDefined();
expect(error!.message).toMatch(`Not supported SqlPackage action: '${actionName}'`);
expect(getSqlPackagePathSpy).toHaveBeenCalledTimes(1);
});
});
});

/**
Expand Down Expand Up @@ -168,7 +209,49 @@ function getInputs(actionType: ActionType, connectionString: string = '') {
actionType: ActionType.BuildAndPublish,
connectionConfig: config,
filePath: './TestProject.sqlproj',
buildArguments: '--verbose --test "test value"'
buildArguments: '--verbose --test "test value"',
sqlpackageAction: SqlPackageAction.Publish
} as IBuildAndPublishInputs
}
}
}

/**
* Gets test inputs used by SQL action based on actionType. Also accepts a custom SqlpackageAction type and additional arguments.
* @param actionType The action type used for testing
* @param sqlpackageAction The custom sqlpackage action type to test
* @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 = '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we specify the return type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const defaultConnectionConfig = new SqlConnectionConfig('Server=testServer.database.windows.net;Initial Catalog=testDB;User Id=testUser;Password=placeholder');

switch(actionType) {
case ActionType.DacpacAction: {
return {
actionType: ActionType.DacpacAction,
connectionConfig: defaultConnectionConfig,
filePath: './TestPackage.dacpac',
sqlpackageAction: sqlpackageAction,
additionalArguments: additionalArguments
} as IDacpacActionInputs;
}
case ActionType.SqlAction: {
return {
actionType: ActionType.SqlAction,
connectionConfig: defaultConnectionConfig,
filePath: './TestFile.sql',
additionalArguments: additionalArguments
} as IActionInputs;
}
case ActionType.BuildAndPublish: {
return {
actionType: ActionType.BuildAndPublish,
connectionConfig: defaultConnectionConfig,
filePath: './TestProject.sqlproj',
buildArguments: '--verbose --test "test value"',
sqlpackageAction: sqlpackageAction,
additionalArguments: additionalArguments
} as IBuildAndPublishInputs
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/main.js
View file
Open in desktop

Large diffs are not rendered by default.

27 changes: 17 additions & 10 deletions src/AzureSqlAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export interface IBuildAndPublishInputs extends IActionInputs {
}

export enum SqlPackageAction {
// Only the Publish action is supported currently
Publish,
Extract,
Export,
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any invalid inputs should be handled by this point

const dacpacPath = await this._executeBuildProject(buildAndPublishInputs);

// Reuse DacpacAction for publish
const publishInputs = {
actionType: ActionType.DacpacAction,
connectionConfig: this._inputs.connectionConfig,
connectionConfig: buildAndPublishInputs.connectionConfig,
filePath: dacpacPath,
additionalArguments: this._inputs.additionalArguments,
sqlpackageAction: SqlPackageAction.Publish
additionalArguments: buildAndPublishInputs.additionalArguments,
sqlpackageAction: buildAndPublishInputs.sqlpackageAction
} as IDacpacActionInputs;
await this._executeDacpacAction(publishInputs);
}
Expand Down Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

case SqlPackageAction.Script:
case SqlPackageAction.DeployReport:
case SqlPackageAction.DriftReport:
args += `/Action:${SqlPackageAction[inputs.sqlpackageAction]} /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}"`;
break;
}
default: {

// When action isn't specified, default to Publish
case undefined:
args += `/Action:Publish /TargetConnectionString:"${inputs.connectionConfig.ConnectionString}" /SourceFile:"${inputs.filePath}"`;
break;

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

if (!!inputs.additionalArguments) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Expand Down
19 changes: 18 additions & 1 deletion src/AzureSqlActionHelper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,28 @@ export default class AzureSqlActionHelper {
}

public static getSqlpackageActionTypeFromString(action: string): SqlPackageAction {
// Default to Publish if not specified
if (!action) {
return SqlPackageAction.Publish;
}

switch (action.trim().toLowerCase()) {
case 'publish':
return SqlPackageAction.Publish;
// case 'extract':
// return SqlPackageAction.Extract;
// case 'import':
// return SqlPackageAction.Import;
// case 'export':
// return SqlPackageAction.Export;
case 'driftreport':
return SqlPackageAction.DriftReport;
case 'deployreport':
return SqlPackageAction.DeployReport;
case 'script':
return SqlPackageAction.Script;
default:
throw new Error(`Action ${action} is invalid. Supported action types are: Publish.`);
throw new Error(`Action ${action} is invalid. Supported action types are: Publish, Script, DriftReport, or DeployReport.`);
}
}

Expand Down