-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Segregate Docker-dependent tests to GitHub Actions workflow #49481
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
500493f
bbcbe63
4305a83
17be87a
15d5972
986471e
e8344ea
867a0f2
e7484cb
acbb292
154c419
308b1a4
c20b498
eb1c3e0
4f0bb67
1390185
9149e64
0c1cf4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| name: Spring Docker Tests | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - '.github/workflows/spring-docker-tests.yml' | ||
| - 'pom.xml' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this really trigger on every PR that changes any pom.xml?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not every pom.xml, it's just the root pom.xml: https://github.com/Azure/azure-sdk-for-java/blob/main/pom.xml |
||
| - 'sdk/spring/**' | ||
| - 'sdk/parents/**' | ||
|
Copilot marked this conversation as resolved.
Outdated
|
||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| spring-docker-tests: | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 45 | ||
|
rujche marked this conversation as resolved.
Outdated
|
||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
rujche marked this conversation as resolved.
Outdated
|
||
|
|
||
| - name: Set up JDK 17 | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '17' | ||
| cache: maven | ||
|
|
||
| - name: Verify Docker | ||
| run: | | ||
| docker version | ||
| docker info | ||
|
|
||
| - name: Build spring docker test dependencies | ||
| run: > | ||
| mvn -f sdk/spring/pom.xml | ||
| -pl spring-cloud-azure-testcontainers,spring-cloud-azure-docker-compose | ||
| -am | ||
| install | ||
| -DskipTests | ||
| -DskipITs | ||
| -DfailIfNoTests=false | ||
|
rujche marked this conversation as resolved.
|
||
|
|
||
| - name: Run spring docker-tagged tests | ||
| run: > | ||
| mvn -f sdk/spring/pom.xml | ||
| -pl spring-cloud-azure-testcontainers,spring-cloud-azure-docker-compose | ||
| test | ||
| -Pdocker-tests | ||
| -DfailIfNoTests=false | ||
| -DskipITs | ||
| -DskipTests=false | ||
|
mikeharder marked this conversation as resolved.
Outdated
|
||
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.
Would this workflow be required check for pull requests?
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.
It's a repo level setting instead of GitHub work flow setting. Here is the detailed steps I got from Copilot:
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 knew it's branch policy settings. My question was that would you expect to set this workflow as required for PRs targeting the
mainbranch?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.
Yes. I don't have permission to configure the branch policy, could you please help to set it?
Uh oh!
There was an error while loading. Please reload this page.
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.
required checks are tricky, because then they need to run on every PR and give a green no-op. maybe better to keep this optional and hope people just don't merge with it failing?
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.
Not every PR, only PRs that modified related files.
What does this mean?
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.
If a check is required in github, PRs can only merge if the check is run, and is passing. So any required check needs to run on every PR (not just a subset of changed files), and it must pass if it doesn't apply. However, we generally don't want to do this, it wastes resources and clutters the UI on every PR.
Instead, you can either live with the check being optional for now. Or you will need to wait until we have a new app-based check-enforcer meta-check that @raych1 is starting to work on.
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.
@mikeharder Thanks for the detailed explanation.
I can accept this.