-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
DAT-15775 #4780
DAT-15775 #4780
Conversation
776d16d
to
53b7302
Compare
3352a95
to
6fec875
Compare
This reverts commit c75d518.
…compareGeneratedSqlWithExpectedSqlForMinimalChangesets/alterSequence/mariadb.sql Signed-off-by: filipe <flautert@liquibase.org>
…compareGeneratedSqlWithExpectedSqlForMinimalChangesets/createSequence/mariadb.sql Signed-off-by: filipe <flautert@liquibase.org>
…compareGeneratedSqlWithExpectedSqlForMinimalChangesets/dropSequence/mariadb.sql Signed-off-by: filipe <flautert@liquibase.org>
…compareGeneratedSqlWithExpectedSqlForMinimalChangesets/renameSequence/mariadb.sql Signed-off-by: filipe <flautert@liquibase.org>
latest_merge_sha=`(git rev-parse HEAD)` | ||
echo "latestMergeSha=${latest_merge_sha}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this will get us the sha we are looking for. We've had trouble using this kind of thing in the past, where it gave us commits that didn't exist, but that may have been so long ago that it's not a problem any more, particularly if we used to build with pull_request_target and now we don't (or vice versa). It's something to keep an eye on.
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
cancel-in-progress: true | ||
group: ${{ github.workflow }}-${{ github.head_ref && github.ref || github.run_id }} | ||
cancel-in-progress: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is cancel-in-progress supposed to be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check on this again. I remember I had to do this for testing.
## verify that the contents of the tar matches the expectation | ||
echo "Comparing the contents of the tar.gz with the expected contents (using baseline file in liquibase-dist directory) to ensure that no new files were accidentally added. A failure here indicates that a new file was added to the tar.gz, and it should either be rectififed or added to the baseline." | ||
#echo "Comparing the contents of the tar.gz with the expected contents (using baseline file in liquibase-dist directory) to ensure that no new files were accidentally added. A failure here indicates that a new file was added to the tar.gz, and it should either be rectififed or added to the baseline." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest you uncomment this echo
} | ||
run-functional-tests: | ||
needs: [ setup, build_publish_branch ] | ||
uses: liquibase/liquibase/.github/workflows/run-functional-tests.yml@github-action-DAT-15775 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this line have @github-action-DAT-15775
in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was taken care of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the steps in this workflow be commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The azure uber jar was creating issue during last release as well. I did not want to look into it for this ticket and go down that rabbit hole.
- name: Report PRO-Tests Run URL | ||
uses: actions/github-script@v6.4.1 | ||
with: | ||
script: | | ||
const targetURL = "https://github.com/liquibase/liquibase-pro-tests/actions/runs/${{ steps.return_dispatch.outputs.run_id }}" | ||
core.notice("Pro-Tests Run URL: " + targetURL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition, nice job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially the same exact file as .github/workflows/run-functional-tests.yml
and these could probably be combined and made more dynamic.
Impact
build.yml
: removing the use of workflow-helper.js frombuild.yml
run-tests.yml
: consist ofunit-tests
andintegration-tests
. If the PR is from a forked repo, only authorize will allow the workflow to run.build.yml
: build and publishes packages to build artifact page and GPMfunctional-tests
andtests-harness
: are run after the newbuild.yml
completes successfully. The link is visible on the run of run-tests.ymlDescription
Things to be aware of
Things to worry about
Additional Context