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

Add ProtocolVersion to StandardDefs #16237

Merged

Conversation

gosusnp
Copy link
Contributor

@gosusnp gosusnp commented Sep 1, 2022

What

Enable storing and reading protocol_versions in actor definitions.

How

Add version to StandardDestinationDefinition and StandardSourceDefinition.
Propagates to the database operations

Closes #16201

This updates the database read/write operations
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -55,3 +55,6 @@ properties:
format: date
resourceRequirements:
"$ref": ActorDefinitionResourceRequirements.yaml
protocol_version:
Copy link
Contributor

Choose a reason for hiding this comment

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

just passing by, seems like we're using camelCase for most of these - should it be protocolVersion here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I think I saw a few comments about prefering snake_case over camelCase for the attributes in the models.
The generated output remains camelCase for the Java objects.
No strong opinion here.

Copy link
Contributor

@pedroslopez pedroslopez Sep 1, 2022

Choose a reason for hiding this comment

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

Ah yeah - main reason why I pointed this out is since at least specifically within these StandardXYZDefinitions schemas it seems like camel case is being used. Also no strong opinion, just think we should try to be consistent

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go for file consistency there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same conclusion, change on the way

@gosusnp gosusnp temporarily deployed to more-secrets September 1, 2022 21:01 Inactive
@gosusnp gosusnp temporarily deployed to more-secrets September 1, 2022 21:30 Inactive
@gosusnp gosusnp temporarily deployed to more-secrets September 1, 2022 22:49 Inactive
@gosusnp gosusnp temporarily deployed to more-secrets September 2, 2022 00:22 Inactive
@gosusnp gosusnp merged commit c8899a1 into master Sep 2, 2022
@gosusnp gosusnp deleted the gosusnp/16201-add_protocol_version_to_actor_defs_apis branch September 2, 2022 16:29
letiescanciano added a commit that referenced this pull request Sep 5, 2022
* master: (47 commits)
  Add email to identify users analytics call (#16327)
  🎉 Source Amazon Ads: improve `config.start_date` validation (#16191)
  Add comments about intermediate state emission (#16262)
  MySQL Source : Standardize spec.json for DB connectors that support log-based CDC replication (#16216)
  MSSQL Source : Standardize spec.json for DB connectors that support log-based CDC replication (#16215)
  Hide a bunch more destination with potential unsecure API access (#16320)
  Skip unit tests when run-tests is false (#16267)
  Hide Destination connections from UI (#16310)
  Add scheduled task to clean up old files from workspace (#16247)
  Source Google Analytics v4: Re-name google analytics connector (#16306)
  🐛 Source Facebook Marketing: remove "end_date" from config if empty value (re-implement #16096) (#16222)
  Fix github action syntax (#16277)
  Re-name google analytics cionnectors (#16287)
  Bump Airbyte version from 0.40.3 to 0.40.4 (#16275)
  Hide ES and Redis destination connectors from Cloud (#16276)
  15700  add tests for PokeAPI (#15701)
  Add ProtocolVersion to StandardDefs (#16237)
  🪟 🔧 🧹 Migrate attempt `bytesSynced` to `totalStats.bytesEmitted` and cleanup `AttemptDetails` component (#16126)
  Improve behavior of password input field (#16011)
  Improve airbyte-metrics support in the Helm chart (#16166)
  ...
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* Add ProtocolVersion to StandardDefs

This updates the database read/write operations

* Fix letter case
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* Add ProtocolVersion to StandardDefs

This updates the database read/write operations

* Fix letter case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add protocol_version to StandardSourceDefinitions and StandardDestinationDefinitions
4 participants