fix: remove invalid steps from airbyte-ci test options #38246
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @erohmensing and the rest of your teammates on |
erohmensing
force-pushed
the
ella/invalidsteps
branch
from
88768c8 to
fdd3834
Compare
There was a problem hiding this comment.
Thanks for cleaning this out. It's something I should have done while migrating these checks inside the connectors_qa package.
| @@ -208,7 +208,10 @@ def _filter_skipped_steps(steps_to_evaluate: STEP_TREE, skip_steps: List[str], r | |||
|
|
|||
| else: | |||
| steps_to_run.append(step_to_eval) | |||
|
|
|||
| if not steps_to_run: | |||
| raise InvalidStepConfiguration( | |||
There was a problem hiding this comment.
nit: I'm not sure we shall raise an exception here.
steps_to_run are currently static: they apply to all connectors selected by airbyte-ci connectors.
In #38281 I'm slightly changing this logic, some connectors might run different steps according to the state of their metadata file.
It might conceptually mean that it's fine to have empty steps_to_run.
In any case that's not too problematic to raise ATM as step trees still always contains static steps to run by default (e.g. build, qa checks etc.)
There was a problem hiding this comment.
It might conceptually mean that it's fine to have empty steps_to_run.
I'm sure y'all have had the discussion about if that's actually okay to run no tests, so I won't get into that :D
Thanks for the context, makes sense! I'll keep the exception for now, feel free to remove it in that PR.
There was a problem hiding this comment.
Hm. Actually, after trying to fix a unit test, I realized that the recursive looping makes this more complicated. We could check that the results_dict actually has step results in it after everything has executed, but there are many places where we'd have to update, and then decide if its okay if steps are skipped in different places (e.g. maybe migrate-to-poetry is a no-op if all steps are skipped for some valid reason, while i'd always expect test to have steps)
Since you're actively playing with this logic, i'll just remove it
erohmensing
force-pushed
the
ella/invalidsteps
branch
from
77905e8 to
a9db74e
Compare
What
-k metadata_validation) and see it silently succeed with no steps run.How
Can this PR be safely reverted and rolled back?