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

Add DACPAC action test to PR check #63

Merged
merged 14 commits into from
Feb 22, 2022
Merged

Add DACPAC action test to PR check #63

merged 14 commits into from
Feb 22, 2022

Conversation

zijchen
Copy link
Contributor

@zijchen zijchen commented Jan 4, 2022

Looks like we only test the SQL action and not the DACPAC action right now. Added a test for this in the workflow.

DACPAC action should deploy a database with 1 table, and the SQLCMD action should verify it and cleanup afterwards.

@zijchen zijchen temporarily deployed to Automation test January 4, 2022 21:18 Inactive
@zijchen zijchen temporarily deployed to Automation test January 4, 2022 21:18 Inactive
@zijchen zijchen temporarily deployed to Automation test January 4, 2022 21:54 Inactive
@zijchen zijchen temporarily deployed to Automation test January 4, 2022 21:54 Inactive
@zijchen zijchen temporarily deployed to Automation test January 4, 2022 22:16 Inactive
@zijchen zijchen temporarily deployed to Automation test January 4, 2022 22:16 Inactive
@zijchen zijchen temporarily deployed to Automation test January 5, 2022 17:48 Inactive
@zijchen zijchen temporarily deployed to Automation test January 5, 2022 17:48 Inactive
@zijchen zijchen temporarily deployed to Automation test January 5, 2022 18:04 Inactive
@zijchen zijchen temporarily deployed to Automation test January 5, 2022 18:04 Inactive
.github/workflows/pr-check.yml Outdated Show resolved Hide resolved

- uses: ./

# Deploy a DACPAC with only a table to server
Copy link

Choose a reason for hiding this comment

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

does the database get deleted after the test is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently. I looked into dropping it but because this action runs in parallel on Linux and Windows, I ran into race conditions occasionally when either SqlPackage or SQLCMD hasn't finished and the DB is already dropped by the other run.

I'm not sure what's the best way to cleanup here. Maybe we can add a scheduled job to delete the DB at midnight or something.

Copy link

Choose a reason for hiding this comment

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

Is it possible for the database name to be different for Linux and Windows? Otherwise a scheduled job sounds good! It would be good to clean up the database so the subscription doesn't get charged for it when it isn't being used and it's a small dacpac being deployed anyway.

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 test DB name as an env variable. Also added a cleanup step to drop the DB. The cleanup step has to be separate because it needs connection to master DB.

__testdata__/testsql.sql Outdated Show resolved Hide resolved
@zijchen zijchen temporarily deployed to Automation test January 11, 2022 18:36 Inactive
@zijchen zijchen temporarily deployed to Automation test January 11, 2022 18:36 Inactive
@zijchen zijchen temporarily deployed to Automation test January 11, 2022 18:44 Inactive
@zijchen zijchen temporarily deployed to Automation test January 11, 2022 18:44 Inactive
@zijchen
Copy link
Contributor Author

zijchen commented Jan 12, 2022

After merging with master branch, PR check fails because #73 changed the target to pull-request-target which uses the yml definition from master branch, but with sources from the PR branch (no idea why that is). However earlier runs succeed (see deployment history above) so this change should not cause regression.

@dzsquared
Copy link
Collaborator

After merging with master branch, PR check fails because #73 changed the target to pull-request-target which uses the yml definition from master branch, but with sources from the PR branch (no idea why that is). However earlier runs succeed (see deployment history above) so this change should not cause regression.

ah my apologies, I didn't realize this change impacts the yml action definition used by the run, but that makes sense. We can potentially revert that change, although it means all tests for PRs from external forks will fail because they won't have access to the secrets needed. I'll investigate further.

@zijchen
Copy link
Contributor Author

zijchen commented Jan 13, 2022

After merging with master branch, PR check fails because #73 changed the target to pull-request-target which uses the yml definition from master branch, but with sources from the PR branch (no idea why that is). However earlier runs succeed (see deployment history above) so this change should not cause regression.

ah my apologies, I didn't realize this change impacts the yml action definition used by the run, but that makes sense. We can potentially revert that change, although it means all tests for PRs from external forks will fail because they won't have access to the secrets needed. I'll investigate further.

I think it's fine. Maybe after all my PRs that will change the yaml definition (this one and #65) we can reimplement #73. Thoughts?

@github-actions
Copy link

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

@github-actions github-actions bot added the idle Inactive for 14 days label Jan 27, 2022
@zijchen zijchen merged commit 4b44443 into master Feb 22, 2022
@zijchen zijchen deleted the deploy-dacpac branch June 21, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idle Inactive for 14 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants