-
Notifications
You must be signed in to change notification settings - Fork 2.3k
ci: add autoqa migration workflow #6261
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
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to e096c31 in 2 minutes and 37 seconds. Click for details.
- Reviewed
336lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/autoqa-migration.yml:140
- Draft comment:
Job-level 'if' conditions for Ubuntu should reference inputs via github.event.inputs and be wrapped in ${{ }}. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% 1. This is a new workflow file being added. 2. The current syntax usesinputs.Xwhile the suggestion is to usegithub.event.inputs.X. 3. Looking at GitHub Actions docs, both syntaxes actually work -inputsis automatically available in expressions. 4. The ${{ }} wrapper is optional in if conditions since GitHub Actions 2.0. 5. The current code is perfectly functional. The suggestion follows a more explicit style that some prefer, but the current code is valid and works correctly. Am I being too lenient by allowing the shorter syntax? No - GitHub Actions documentation explicitly states that both syntaxes are valid, and we should avoid making style-only changes when the code is functionally correct. The comment should be deleted because it suggests changing valid, working code to an alternative but not necessarily better syntax.
2. .github/workflows/autoqa-migration.yml:248
- Draft comment:
Job-level 'if' condition for macOS should use ${{ github.event.inputs.old_macos_installer != '' && github.event.inputs.new_macos_installer != '' }} to properly reference workflow_dispatch inputs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% 1. Both syntax styles are valid in GitHub Actions. 2. The simpler 'inputs.x' syntax is actually the recommended form in GitHub docs. 3. The suggestion adds unnecessary verbosity without any functional benefit. 4. The existing code is consistent with the Ubuntu job above which uses the same pattern. The suggestion might be trying to be more explicit about the context, which could be seen as more readable by some. While being explicit isn't bad, the simpler syntax is standard, works correctly, and is more maintainable. The comment suggests a more verbose syntax that provides no functional benefit over the existing correct and standard syntax.
3. .github/workflows/autoqa-migration.yml:53
- Draft comment:
The Python setup uses version 3.13 which is cutting edge. Verify that all runners support it or consider using a stable version. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify compatibility with all runners, which is against the rules. It also suggests considering using a stable version, but this is not a specific code suggestion or a request for a test. Therefore, it should be removed.
4. .github/workflows/autoqa-migration.yml:112
- Draft comment:
Ensure consistent quoting/escaping of the test case argument across platforms to avoid shell parsing issues (e.g., PowerShell vs. Bash). - Reason this comment was not posted:
Comment looked like it was already resolved.
5. .github/workflows/autoqa-migration.yml:66
- Draft comment:
Consider adding error handling/logging in the installer download steps to catch failures before copying files. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% 1. This is a new file being added, so the comment is about changed code. 2. Error handling is generally important for robustness. 3. However, without seeing the download scripts' implementation, we don't know if error handling already exists. 4. The comment is somewhat vague about what specific error handling is needed. 5. GitHub Actions will show errors if commands fail, providing some basic error visibility. I may be underestimating the importance of explicit error handling in CI/CD pipelines. Failed downloads could lead to confusing errors later in the workflow. While error handling is important, the comment doesn't provide enough specifics about what's missing, and we can't verify there's actually a problem without seeing the download scripts. The comment should be removed because it's speculative without evidence of missing error handling, and isn't specific enough about what changes are needed.
Workflow ID: wflow_6fARkKdLJl0DwQjI
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| type: number | ||
| default: 65 | ||
|
|
||
| jobs: |
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.
Common steps like checkout, Python setup, and dependency installation are duplicated across jobs. Consider refactoring with reusable actions or templates.
This pull request introduces a new GitHub Actions workflow,
.github/workflows/autoqa-migration.yml, to enable manual migration testing of installers across Windows, Ubuntu, and macOS. The workflow allows testers to specify old and new installer paths/URLs and optionally select specific migration test cases. It automates environment setup, installer downloads, test execution, artifact uploads, and cleanup for each supported OS.Key additions and improvements:
New workflow for migration testing:
.github/workflows/autoqa-migration.ymlto provide a manual workflow for running migration tests on Windows, Ubuntu, and macOS, supporting custom installer inputs and test case selection.Per-OS migration test jobs:
autoqaframework, and post-test cleanup.Parameterization and flexibility:
Important
Adds a GitHub Actions workflow for manual migration testing on Windows, Ubuntu, and macOS with customizable inputs and test case selection.
.github/workflows/autoqa-migration.ymlfor manual migration testing on Windows, Ubuntu, and macOS.This description was created by
for e096c31. You can customize this summary. It will automatically update as commits are pushed.