-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Airbyte-ci: Add skippable connector test steps #32188
Conversation
…options-for-steps
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
2cb24a3
to
258b66d
Compare
aa29862
to
3505170
Compare
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 LOVE the direction where this is going.
@click.option( | ||
'--skip-step', | ||
'-x', | ||
multiple=True, | ||
type=str, | ||
help="Skip a step by name. Can be used multiple times to skip multiple steps.", | ||
) |
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.
How big of a lift would it be to have an --only
option to select a specific test suite?
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 much at all! I can do that in another PR
Issue here: https://github.com/airbytehq/airbyte/issues/33309
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/commands.py
Outdated
Show resolved
Hide resolved
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/commands.py
Outdated
Show resolved
Hide resolved
async def connector_secrets(self): | ||
if self._connector_secrets is None: | ||
self._connector_secrets = await secrets.get_connector_secrets(self) | ||
return self._connector_secrets | ||
|
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.
Is this related to your changes?
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 is! We were mounting connector secrets in between steps. So I hoisted that logic into the context so when you request connector_secrets
it mounts and gets them if it hasnt already
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.
Could we rename the function to get_connector_secrets
then? Or would it make sense to make secrets.get_connector_secrets
a method of the context class?
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.
Im torn on this
On one hand it belongs in context
given how coupled it is to a ConnectorContext
On the other hand connector secrets is a foundational concept that deserves a separate file to hold onto the logic.
I think we gain more from it being separate.
WDYT?
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 suggest renaming it to get_connector_secrets
as it:
- Downloads the connector secrets
- Assign the secrets to
self._connector_secrets
.
And we rename self._connector_secrets
to self.connector_secrets
to make it public.
I'd be confused to have a method named connector_secrets
acting as a getter on self._connector_secrets
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.
Sounds good! Though, lets leave the internal field with the underscore. We want devs to always reference the getter and feel bad otherwise
args=lambda results: {"connector_under_test": results["build"].output_artifact[LOCAL_BUILD_PLATFORM]}, | ||
depends_on=["build"], |
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 I'm not mistaken the dependency is always implicitly defined by the fact that a Step args are from a dependency output_artifact
. Could we infer the dependency tree dynamically from the args then?
I know explicit is better than implicit 😄 , so feel free to reject this suggestion
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.
Thats fair! but there sometimes output that may not be an artifact, thus theres no implicit relationship either.
e.g. we have a step called PushToDockerHub
and a step called VerifyOnDockerHub
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.
Could you add tests that check that argument passing between steps works?
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/test/steps/java_connectors.py
Outdated
Show resolved
Hide resolved
id: str | ||
step: Step | ||
args: ARGS_TYPE = field(default_factory=dict) | ||
depends_on: List[str] = field(default_factory=list) |
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 would find it more correct and robust if the depends_on
type was a list of StepToRun
instances
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.
Im not full against that but why is that?
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.
A child could know its parents if depends_on
was a StepToRun
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.
And it limits id typos as we'd pass around a variable
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.
So with the need of having --help
list the name of available steps to skip We've added the pattern where steps id's are defined as constants and then reused.
That should fix the typo problem you pointed out here.
Which I think is a good approach since we dont want to pass around fully instantiated StepToRun
objects as that would lead us into a couple of painful areas
- Passing the
BuildStepInstance
everywhere - Preventing us from having a polymorphic build step. e.g.
BuildStepInstance
orBuildWithNormalizationStepInstance
WDYT?
|
||
|
||
@dataclass(frozen=True) | ||
class StepToRun: |
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.
Just opening the discussion here.
Could we rename StepToRun
to Step
And Step
to Operation
?
A Operation.__init__
would take a Step
instance and would access parameters from Step.args
.
Step
instances would be "singleton"
Operation
instance would not be singleton: an operation can be re-used in multiple step.
And I think Operation
might be worth being functions as they'll be stateless.
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.
Im all for finding a way to cut down the awkwardness (and layers) of these abstraction.
Hearing works like "singleton", "would be" and "I think" is telling me that this might be better tackled on its own so we can just wrestle with restructuring the taxonomy and not restructuring the taxonomy AND adding new functionality.
WDYT?
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.
Sure!
@alafanechere This is ready for review again! Since the last review
I also left a few discussions open around What im worried about is that I may have clobbered a change we've made in the last month. I havent found any regression but would love your eagle eyes on that! |
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 think there's a bug in the input validation:
airbyte-ci connectors --name=source-faker test -x=acceptance
:
┃ Error: Invalid value for '--skip-step' / '-x': '=acceptance' is not one of <CONNECTOR_TEST_STEP_ID.ACCEPTANCE: 'acceptance'>, <CONNECTOR_TEST_STEP_ID.BUILD_NORMALIZATION: 'build_normalization'>, <CONNECTOR_TEST_STEP_ID.BUILD_TAR: 'build_tar'>, <CONNECTOR_TEST_STEP_ID.BUILD: 'build'>, <CONNECTOR_TEST_S
┃ TEP_ID.CHECK_BASE_IMAGE: 'check_base_image'>, <CONNECTOR_TEST_STEP_ID.INTEGRATION: 'integration'>, <CONNECTOR_TEST_STEP_ID.METADATA_VALIDATION: 'metadata_validation'>, <CONNECTOR_TEST_STEP_ID.QA_CHECKS: 'qa_checks'>, <CONNECTOR_TEST_STEP_ID.UNIT: 'unit'>, <CONNECTOR_TEST_STEP_ID.VERSION_FOLLOW_CHECK:
┃ 'version_follow_check'>, <CONNECTOR_TEST_STEP_ID.VERSION_INC_CHECK: 'version_inc_check'>, <CONNECTOR_TEST_STEP_ID.TEST_ORCHESTRATOR: 'test_orchestrator'>, <CONNECTOR_TEST_STEP_ID.DEPLOY_ORCHESTRATOR: 'deploy_orchestrator'>.
…options-for-steps
@alafanechere Ready for your review again :) |
@bnchrch I figured that |
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.
Ship it 🎉
I think I fixed the test suite + conflicts.
I manually tested connectors test
, everything is working as expected so far!
Co-authored-by: Augustin <augustin@airbyte.io> Co-authored-by: bnchrch <bnchrch@users.noreply.github.com> Co-authored-by: alafanechere <augustin.lafanechere@gmail.com>
Co-authored-by: Augustin <augustin@airbyte.io> Co-authored-by: bnchrch <bnchrch@users.noreply.github.com> Co-authored-by: alafanechere <augustin.lafanechere@gmail.com>
Co-authored-by: Augustin <augustin@airbyte.io> Co-authored-by: bnchrch <bnchrch@users.noreply.github.com> Co-authored-by: alafanechere <augustin.lafanechere@gmail.com>
Problem
As a connector developer I would like to be able to skip individual steps in the test pipeline
Proposed Solution
Implementation
Defining the Steps to be run as a list
Other minor changes
Why a list?
This will give us a type and structure we can manupilate prior to running, enabling features like
Why a new class?
The new class
StepToRun
was added for two reasonsrun
still would require an addtional class / datamodelrun_steps.py
file