Feature/831 no longer overwrite slow checks#834
Conversation
…; update other workflows
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
had a problem deploying
to
manual-approval
GitHub Actions
Error
| ) | ||
|
|
||
| try: | ||
| validate_workflow_name(workflow_name, allow_not_maintained=is_new_project) |
There was a problem hiding this comment.
I have the feeling, that passing this option to validate_workflow_name() and later on catching the exception NotMaintainedWorkflowError is somewhat complicated / redundant and awkward.
Could we maybe
- not pass arg
allow_not_maintainedtovalidate_workflow_name() - in
except NotMaintainedWorkflowError: check if it is a new project and only print the log message for old projects
|
|
||
| underlying_error = ex.value.__cause__ | ||
| assert isinstance(underlying_error, ValidationError) | ||
| assert "workflows.0.name" in str(underlying_error) |
There was a problem hiding this comment.
Shouldn't this test verify NOT_MAINTAINED_WORKFLOW_NAMES[0] rather than "workflows.0.name"?
There was a problem hiding this comment.
Also, I am not sure whether the name of the test matches the content.
Shouldn't not_maintained_workflow_name raise an NotMaintainedWorkflowError rather than InvalidWorkflowPatcherYamlError?
There was a problem hiding this comment.
In this test, because it involves the pydantic model, the ValidationError is the one causing the issue.
So we have:
- NotMaintainedWorkflowError -> this is our actual issue
- This is caught internally by pydantic which raises a ValidationError -> this is just pydantic being pydantic. It usually does this as the model might have multiple errors, and we don't want to break on just the 1st one.
- We catch this and raise an InvalidWorkflowPatcherYamlError -> this tells you which file had issues
There was a problem hiding this comment.
I ended up modifying the exception to display some more information, as in another issue, it was stated that it got cut off.
| @staticmethod | ||
| @pytest.mark.parametrize("workflow_name", NOT_MAINTAINED_WORKFLOW_NAMES) | ||
| def test_works_as_expected_for_old_project_where_not_maintained_workflows_skipped( | ||
| project_config_without_patcher, workflow_name |
There was a problem hiding this comment.
Shouldn't this test maybe verify, that the file content remains unchanged?
There was a problem hiding this comment.
I can modify the test to do that.
My purpose here was to show that if an old project exists, which is determined by any *.yml being present in the .github/workflows directory, that slow-checks.yml wouldn't be added to it.
ArBridgeman
requested a deployment
to
manual-approval
GitHub Actions
Waiting
|
3 participants
closes #831
Checklist
Note: If any of the items in the checklist are not relevant to your PR, just check the box.
For any Pull Request
Is the following correct:
When Changes Were Made
Did you:
When Preparing a Release
Have you: