Skip to content
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

CDK: Fix regression in _checkpoint_state arg #16141

Merged
merged 7 commits into from Aug 31, 2022

Conversation

grubberr
Copy link
Contributor

@grubberr grubberr commented Aug 30, 2022

Signed-off-by: Sergey Chvalyuk grubberr@gmail.com

What

Fix incorrect use of self._checkpoint_state
we need to pass stream_state because stream_instance.state can be undefined

Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
@grubberr grubberr requested a review from a team as a code owner August 30, 2022 18:59
@grubberr grubberr self-assigned this Aug 30, 2022
@github-actions github-actions bot added the CDK Connector Development Kit label Aug 30, 2022
@grubberr grubberr changed the title CDK: fix regression in PR #15067 CDK: Fix regression in _checkpoint_state arg Aug 30, 2022
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

@grubberr can you edit the PR description to include a description of the problem, an example, and why this code change fixes it?

@edgao
Copy link
Contributor

edgao commented Aug 30, 2022

@sherifnada I think the context is https://github.com/airbytehq/oncall/issues/466

I don't think I can competently review this PR, I have basically no experience with CDK internals; removing my review request. LMK if there's anything I can do to help get this deployed though.

couple drive-by comments while I'm here:

  • is the plan to publish source-amplitude as part of this pr (and/or in a followup pr)?
  • is it possible to add a unit test to catch whatever the problem was?

@edgao edgao removed their request for review August 30, 2022 23:22
@grubberr
Copy link
Contributor Author

grubberr commented Aug 31, 2022

/publish-cdk dry-run=true

https://github.com/airbytehq/airbyte/actions/runs/2962320722

@grubberr
Copy link
Contributor Author

grubberr commented Aug 31, 2022

FYI @sherifnada @lazebnyi @bazarnov @davydov-d @roman-yermilov-gl

New pydantic released 1.10.0 and it's behaviour changed for const keyword

from pydantic import BaseModel, Field

class Choice1(BaseModel):
    field = Field("option1", const=True)

pydantic==1.9.2

{
    "properties": {
        "field": {"title": "Field", "const": "option1", "type": "string"}
    }
}

pydantic==1.10.0

{
    "properties": {
        "field": {"title": "Field", "default": "option1", "const": "option1", "type": "string"}
    }
}

Maybe we would need to return default keyword to common properties in spec again

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py#L122

@davydov-d
Copy link
Collaborator

FYI @sherifnada @lazebnyi @bazarnov @davydov-d @roman-yermilov-gl

New pydantic released 1.10.0 and it's behaviour changed for const keyword

from pydantic import BaseModel, Field

class Choice1(BaseModel):
    field = Field("option1", const=True)

pydantic==1.9.2

{
    "properties": {
        "field": {"title": "Field", "const": "option1", "type": "string"}
    }
}

pydantic==1.10.0

{
    "properties": {
        "field": {"title": "Field", "default": "option1", "const": "option1", "type": "string"}
    }
}

Maybe we would need to return default keyword to common properties in spec again

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py#L122

As an option we could pin pydantic version not to change/during the change will be made to all the models in the connectors

@sherifnada
Copy link
Contributor

Yeah we can’t upgrade pydantic or it will cause issues.

@grubberr
Copy link
Contributor Author

grubberr commented Aug 31, 2022

SAT updates for new pydantic #16172

@grubberr
Copy link
Contributor Author

@sherifnada I have pinned pydantic~=1.9.2
we can put this PR #16172 on-hold until we decide how to be with new pydantic

@grubberr
Copy link
Contributor Author

grubberr commented Aug 31, 2022

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/2965915354
https://github.com/airbytehq/airbyte/actions/runs/2965915354

@grubberr
Copy link
Contributor Author

/publish-cdk dry-run=false

2 similar comments
@grubberr
Copy link
Contributor Author

/publish-cdk dry-run=false

@grubberr
Copy link
Contributor Author

/publish-cdk dry-run=false

@grubberr
Copy link
Contributor Author

grubberr commented Aug 31, 2022

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/2966494773
https://github.com/airbytehq/airbyte/actions/runs/2966494773

@grubberr grubberr merged commit 3cfa489 into master Aug 31, 2022
@grubberr grubberr deleted the grubberr/airbyte_cdk-fix-checkpoint_state-2 branch August 31, 2022 19:39
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
Signed-off-by: Sergey Chvalyuk <grubberr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants