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

Extract OAuth API #18818

Merged
merged 57 commits into from
Nov 2, 2022
Merged

Extract OAuth API #18818

merged 57 commits into from
Nov 2, 2022

Conversation

benmoriceau
Copy link
Contributor

What

In order to be prepared for a smoother migration to Micronaut for the server, an effort must be made to break the ConfigurationApi into multiple classes. This PR is the first one of a series of PR to come.

How

Extract all the OauthApi into their own classes

Recommended reading order

No specific order

…tehq/airbyte into bmoric/extract-connection-api
…tehq/airbyte into bmoric/extract-connection-api
…airbyte into bmoric/extract-db-migration-api
…q/airbyte into bmoric/extract-destination-definition-api
…m:airbytehq/airbyte into bmoric/extract-destination-definition-specification-api
@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server normalization labels Nov 1, 2022
@benmoriceau benmoriceau changed the title Bmoric/fix open api tag Extract OAuth API Nov 2, 2022
@benmoriceau benmoriceau temporarily deployed to more-secrets November 2, 2022 16:32 Inactive
@@ -28,8 +28,7 @@ def before_all_tests(request):
"test_type": "ephemeral",
"tmp_folders": temporary_folders,
}
if DestinationType.POSTGRES.value not in destinations_to_test:
destinations_to_test.append(DestinationType.POSTGRES.value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is because I branch out of master for this PR instead of my branch. I don't want to rebase my other PR with master because they are already green and ready to merge. Please ignore this change when reviewing.

import javax.ws.rs.Path;
import lombok.AllArgsConstructor;

@Path("/v1/destination_oauths")
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking that oauths should be plural in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it comes from the generated file:

@Path("/v1/source_oauths")
@Api(description = "the SourceOauth API")
@javax.annotation.Generated(value = "org.openapitools.codegen.languages.JavaJAXRSSpecServerCodegen", date = "2022-11-02T15:07:06.363568-04:00[America/New_York]")
public interface SourceOauthApi {

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.

looks good to me high level, one question though, do we need to split into source/dest oauth or is it just making apparent some previously existing dupl? Those two should be pretty similar under the hood.

Base automatically changed from bmoric/extract-notification-api to master November 2, 2022 19:03
@benmoriceau
Copy link
Contributor Author

looks good to me high level, one question though, do we need to split into source/dest oauth or is it just making apparent some previously existing dupl? Those two should be pretty similar under the hood.

I don't want to change the API. and there is 2 API path for it. I can't let them to be in the same controller because it will have the url path /v1 which will erase the existing ConfigurationApi

@benmoriceau benmoriceau temporarily deployed to more-secrets November 2, 2022 20:37 Inactive
@benmoriceau benmoriceau temporarily deployed to more-secrets November 2, 2022 21:51 Inactive
@benmoriceau benmoriceau merged commit 589f6ef into master Nov 2, 2022
@benmoriceau benmoriceau deleted the bmoric/fix-open-api-tag branch November 2, 2022 22:37
letiescanciano added a commit that referenced this pull request Nov 3, 2022
* master: (38 commits)
  New Source: Gridly (#18342)
  🎉 New Source: Alpha Vantage (#18320)
  ci_integration_test.sh: cut GITHUB_STEP_SUMMARY (#18895)
  🎉 New Source: Datadog [python cdk] (#18150)
  Hide Reject all button in consent dialog (#18596)
  feat: add doc url to track event (#18690)
  fix: install java in oss catalog deploy action (#18887)
  [CI] Speed up check_images_exist (#18873)
  Extract open API (#18879)
  Remove unused interfaces (#18880)
  add action for deploying oss connector catalog to GCS (#18633)
  feat: generate full connector catalog json (#18562)
  Add unsupported_protocol_version column to Connection (#18876)
  Extract OAuth API (#18818)
  update images to have non-transparent background (#18874)
  DiscoverSchema endpoints calculates diff and breaking change (#18571)
  Validate protocol version on connector update (#18639)
  Bmoric/extract notification api (#18812)
  Show version and changelog status for affected connectors (#18845)
  Bmoric/extract logs api (#18621)
  ...
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.

3 participants