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 schema change attrs to connection create #19771

Merged
merged 4 commits into from
Nov 29, 2022

Conversation

alovew
Copy link
Contributor

@alovew alovew commented Nov 23, 2022

Allow users to specify non-breaking schema change preference on Connection Create

@octavia-squidington-iv octavia-squidington-iv added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server labels Nov 23, 2022
@alovew alovew temporarily deployed to more-secrets November 23, 2022 19:42 Inactive
@alovew alovew temporarily deployed to more-secrets November 23, 2022 19:42 Inactive
@alovew alovew force-pushed the anne/auto-detect-schema-create-connection branch from 7be2a88 to 5a59579 Compare November 28, 2022 21:03
@alovew alovew requested a review from a team as a code owner November 28, 2022 21:03
@alovew alovew force-pushed the anne/auto-detect-schema-create-connection branch from 48db276 to e54a3c4 Compare November 28, 2022 21:05
@alovew alovew temporarily deployed to more-secrets November 28, 2022 21:07 Inactive
@alovew alovew temporarily deployed to more-secrets November 28, 2022 21:07 Inactive
@alovew alovew force-pushed the anne/auto-detect-schema-create-connection branch from e195953 to cad6127 Compare November 28, 2022 21:31
@alovew alovew temporarily deployed to more-secrets November 28, 2022 21:33 Inactive
@alovew alovew temporarily deployed to more-secrets November 28, 2022 21:33 Inactive
@alovew alovew temporarily deployed to more-secrets November 28, 2022 22:00 Inactive
@alovew alovew temporarily deployed to more-secrets November 28, 2022 22:01 Inactive
Copy link
Contributor

@gosusnp gosusnp left a comment

Choose a reason for hiding this comment

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

LGTM.
Probably out of scope for this PR, but I'll ask since you may have more context. A long list of duplicated properties like this feels very error prone, how complex do you think it would be to refactor WebBackendConnectionCreate so that it can use ConnectionCreate?

@alovew
Copy link
Contributor Author

alovew commented Nov 29, 2022

I'm really not sure why it was built this way. But it seems we have this pattern for all of our objects that are passed to the WebBackendConnectionHandler - there's a version of the object for WebBackendConnectionHandler (e.g. WebBackendConnectionUpdate, WebBackendConnectionPatch, etc) and then a new object is created to pass to the ConnectionHandler. @benmoriceau do you know why this was done this way?

@alovew alovew temporarily deployed to more-secrets November 29, 2022 00:15 Inactive
@alovew alovew temporarily deployed to more-secrets November 29, 2022 00:15 Inactive
@cgardens
Copy link
Contributor

how complex do you think it would be to refactor WebBackendConnectionCreate so that it can use ConnectionCreate?

we've looked in the past and it's hard to merge the definitions of 2 structs in openapi. we have tried to use allof but apparently that's considered bad practice. 🤷‍♀️

@alovew alovew merged commit 8287362 into master Nov 29, 2022
@alovew alovew deleted the anne/auto-detect-schema-create-connection branch November 29, 2022 00:53
@gosusnp
Copy link
Contributor

gosusnp commented Nov 29, 2022

how complex do you think it would be to refactor WebBackendConnectionCreate so that it can use ConnectionCreate?

we've looked in the past and it's hard to merge the definitions of 2 structs in openapi. we have tried to use allof but apparently that's considered bad practice. 🤷‍♀️

What about nesting? (like having WebBackendConnectionCreate contain a ConnectionCreate) It may not be as elegant but I'd say it would be probably more practical for us.

@cgardens
Copy link
Contributor

sure. given our new public api strategy, i feel like we can be a little less precious about these webbackend apis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants