Skip to content

(fix) invalid workflows by commenting #9

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

(fix) invalid workflows by commenting #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 9 additions & 7 deletions .github/scripts/check_diff.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,9 @@ def add_dependents(dirs_to_eval: Set[str], dependents: dict) -> List[str]:


def _get_configs_for_single_dir(job: str, dir_: str) -> List[Dict[str, str]]:
if job == "test-pydantic":
return _get_pydantic_test_configs(dir_)
# Commenting out missing workflow reference
# if job == "test-pydantic":
# return _get_pydantic_test_configs(dir_)
Comment on lines +116 to +118
Copy link

Choose a reason for hiding this comment

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

medium

Consider adding a comment explaining why these workflows are being commented out, and if there are plans to re-enable them in the future. This will help future developers understand the context of these changes.


if dir_ == "libs/core":
py_versions = ["3.9", "3.10", "3.11", "3.12", "3.13"]
Expand Down Expand Up @@ -214,7 +215,7 @@ def _get_configs_for_multi_dirs(
dirs_to_run["lint"] | dirs_to_run["test"] | dirs_to_run["extended-test"],
dependents,
)
elif job in ["test", "compile-integration-tests", "dependencies", "test-pydantic"]:
elif job in ["test", "compile-integration-tests", "dependencies"]:
Comment on lines 217 to +218
Copy link

Choose a reason for hiding this comment

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

medium

It's good to remove the test-pydantic job from this list since it's no longer a valid job. Ensure that this change aligns with the intended behavior of the script.

dirs = add_dependents(
dirs_to_run["test"] | dirs_to_run["extended-test"], dependents
)
Expand Down Expand Up @@ -321,12 +322,13 @@ def _get_configs_for_multi_dirs(
"extended-tests",
"compile-integration-tests",
"dependencies",
"test-pydantic",
# "test-pydantic",
]
}
map_job_to_configs["test-doc-imports"] = (
[{"python-version": "3.12"}] if docs_edited else []
)
# Commenting out missing workflow
# map_job_to_configs["test-doc-imports"] = (
# [{"python-version": "3.12"}] if docs_edited else []
# )
Comment on lines +328 to +331
Copy link

Choose a reason for hiding this comment

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

medium

Similar to the previous comment, adding a brief explanation for commenting out this section would be beneficial for future maintainers.


for key, value in map_job_to_configs.items():
json_output = json.dumps(value)
Expand Down
56 changes: 29 additions & 27 deletions .github/workflows/check_diffs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ jobs:
extended-tests: ${{ steps.set-matrix.outputs.extended-tests }}
compile-integration-tests: ${{ steps.set-matrix.outputs.compile-integration-tests }}
dependencies: ${{ steps.set-matrix.outputs.dependencies }}
test-doc-imports: ${{ steps.set-matrix.outputs.test-doc-imports }}
test-pydantic: ${{ steps.set-matrix.outputs.test-pydantic }}
# test-doc-imports: ${{ steps.set-matrix.outputs.test-doc-imports }}
# test-pydantic: ${{ steps.set-matrix.outputs.test-pydantic }}
Comment on lines +43 to +44
Copy link

Choose a reason for hiding this comment

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

medium

Adding a comment explaining why these outputs are being commented out would be helpful.

lint:
name: cd ${{ matrix.job-configs.working-directory }}
needs: [ build ]
Expand Down Expand Up @@ -70,31 +70,33 @@ jobs:
python-version: ${{ matrix.job-configs.python-version }}
secrets: inherit

test-pydantic:
name: cd ${{ matrix.job-configs.working-directory }}
needs: [ build ]
if: ${{ needs.build.outputs.test-pydantic != '[]' }}
strategy:
matrix:
job-configs: ${{ fromJson(needs.build.outputs.test-pydantic) }}
fail-fast: false
uses: ./.github/workflows/_test_pydantic.yml
with:
working-directory: ${{ matrix.job-configs.working-directory }}
pydantic-version: ${{ matrix.job-configs.pydantic-version }}
secrets: inherit
# Commenting out missing workflow reference
# test-pydantic:
# name: cd ${{ matrix.job-configs.working-directory }}
# needs: [ build ]
# if: ${{ needs.build.outputs.test-pydantic != '[]' }}
# strategy:
# matrix:
# job-configs: ${{ fromJson(needs.build.outputs.test-pydantic) }}
# fail-fast: false
# uses: ./.github/workflows/_test_pydantic.yml
# with:
# working-directory: ${{ matrix.job-configs.working-directory }}
# pydantic-version: ${{ matrix.job-configs.pydantic-version }}
# secrets: inherit
Comment on lines +73 to +86
Copy link

Choose a reason for hiding this comment

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

medium

Adding a comment explaining why this job is being commented out would be helpful.


test-doc-imports:
needs: [ build ]
if: ${{ needs.build.outputs.test-doc-imports != '[]' }}
strategy:
matrix:
job-configs: ${{ fromJson(needs.build.outputs.test-doc-imports) }}
fail-fast: false
uses: ./.github/workflows/_test_doc_imports.yml
secrets: inherit
with:
python-version: ${{ matrix.job-configs.python-version }}
# Commenting out missing workflow reference
# test-doc-imports:
# needs: [ build ]
# if: ${{ needs.build.outputs.test-doc-imports != '[]' }}
# strategy:
# matrix:
# job-configs: ${{ fromJson(needs.build.outputs.test-doc-imports) }}
# fail-fast: false
# uses: ./.github/workflows/_test_doc_imports.yml
# secrets: inherit
# with:
# python-version: ${{ matrix.job-configs.python-version }}
Comment on lines +88 to +99
Copy link

Choose a reason for hiding this comment

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

medium

Adding a comment explaining why this job is being commented out would be helpful.


compile-integration-tests:
name: cd ${{ matrix.job-configs.working-directory }}
Expand Down Expand Up @@ -159,7 +161,7 @@ jobs:
echo "$STATUS" | grep 'nothing to commit, working tree clean'
ci_success:
Copy link

Choose a reason for hiding this comment

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

medium

Removing test-doc-imports and test-pydantic from the needs list is consistent with the removal of those jobs. This ensures that the ci_success job doesn't wait for jobs that no longer exist.

name: "CI Success"
needs: [build, lint, test, compile-integration-tests, extended-tests, test-doc-imports, test-pydantic]
needs: [build, lint, test, compile-integration-tests, extended-tests]
if: |
always()
runs-on: ubuntu-latest
Expand Down
Loading