Skip to content

Abstract the Postgres management client to allow both single and flexible server#1872

Merged
wwlorey merged 24 commits intomicrosoft:mainfrom
tonybaloney:postgres_patch_update
Jul 8, 2021
Merged

Abstract the Postgres management client to allow both single and flexible server#1872
wwlorey merged 24 commits intomicrosoft:mainfrom
tonybaloney:postgres_patch_update

Conversation

@tonybaloney
Copy link
Contributor

@tonybaloney tonybaloney commented Jun 18, 2021

  • Implement server listing
  • Implement connection string
  • Implement database list
  • Implement provisioning

@tonybaloney tonybaloney requested a review from a team as a code owner June 18, 2021 00:13
@tonybaloney tonybaloney marked this pull request as draft June 18, 2021 00:13
@tonybaloney
Copy link
Contributor Author

screenshot 2021-06-18 at 11 05 04

server listing (both single and flexible)

@tonybaloney
Copy link
Contributor Author

The provisioning for flexible creates the server but then hits this issue--

3:34:09 PM: Creating PostgreSQL Server "ant-flexi-2"... It should be ready in several minutes.
3:34:44 PM: Error: The resource type 'locations/azureAsyncOperation' could not be found in the namespace 'Microsoft.DBforPostgreSQL' for api version '2021-04-10-privatepreview'. The supported api-versions are '2017-12-01-preview,2017-12-01,2018-03-29-privatepreview,2020-10-05-privatepreview,2020-02-14-privatepreview,2020-02-14-preview,2020-11-05-preview'.

@tonybaloney
Copy link
Contributor Author

Again for deleting single servers

The resource type 'locations/azureAsyncOperation' could not be found in the namespace 'Microsoft.DBforPostgreSQL' for api version '2021-04-10-privatepreview'. The supported api-versions are '2017-12-01-preview,2017-12-01,2018-03-29-privatepreview,2020-10-05-privatepreview,2020-02-14-privatepreview,2020-02-14-preview,2020-11-05-preview'.

@tonybaloney tonybaloney marked this pull request as ready for review June 18, 2021 07:23
@tonybaloney tonybaloney changed the title [WIP] Abstract the Postgres management client to allow both single and flexible server Abstract the Postgres management client to allow both single and flexible server Jun 18, 2021
Copy link
Contributor

@wwlorey wwlorey left a comment

Choose a reason for hiding this comment

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

The provisioning for flexible creates the server but then hits this issue--

3:34:09 PM: Creating PostgreSQL Server "ant-flexi-2"... It should be ready in several minutes.
3:34:44 PM: Error: The resource type 'locations/azureAsyncOperation' could not be found in the namespace 'Microsoft.DBforPostgreSQL' for api version '2021-04-10-privatepreview'. The supported api-versions are '2017-12-01-preview,2017-12-01,2018-03-29-privatepreview,2020-10-05-privatepreview,2020-02-14-privatepreview,2020-02-14-preview,2020-11-05-preview'.

I'm running into this as well. Do you know if/when that api version will be supported?

@tonybaloney
Copy link
Contributor Author

@wwlorey updates completed from the review

@wwlorey
Copy link
Contributor

wwlorey commented Jun 28, 2021

This looks good 👍👍 I'll wait until we hear back about the API problem before testing again & getting this merged

@tonybaloney
Copy link
Contributor Author

This looks good 👍👍 I'll wait until we hear back about the API problem before testing again & getting this merged

There is an issue with the SDK release. I'm waiting for a patch and will update packages.json once it's done

Copy link
Contributor

@ejizba ejizba left a comment

Choose a reason for hiding this comment

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

Sorry for jumping in here a little late, but I think there's too much duplicated code inside AbstractPostgresClient and models.ts. We lose a bit of type safety and risk some silly copy/paste bugs. We should be able to leverage TypeScript features like union types more heavily to clean it up

@tonybaloney
Copy link
Contributor Author

Sorry for jumping in here a little late, but I think there's too much duplicated code inside AbstractPostgresClient and models.ts. We lose a bit of type safety and risk some silly copy/paste bugs. We should be able to leverage TypeScript features like union types more heavily to clean it up

@ejizba thanks for the feedback. My TS knowledge is still pretty basic. This PR is still blocked on an SDK patch so I'll work through your feedback and update this PR.

@tonybaloney
Copy link
Contributor Author

@ejizba refactored as per your feedback in bb01e7a

@tonybaloney
Copy link
Contributor Author

Blocked by pending update to Azure/azure-sdk-for-js#15979

@tonybaloney
Copy link
Contributor Author

Related Azure/azure-sdk-for-js#16136

@tonybaloney
Copy link
Contributor Author

@wwlorey @ejizba this PR is now finished/ready for review. the backend dependency on the JS SDK has been fixed and I've updated package.json with the new version

@tonybaloney
Copy link
Contributor Author

@ejizba amendments made in cb353d4

Copy link
Contributor

@ejizba ejizba 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! @wwlorey were you still going to double check?

Copy link
Contributor

@wwlorey wwlorey 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 👍

@wwlorey wwlorey merged commit ffd7f44 into microsoft:main Jul 8, 2021
@tonybaloney tonybaloney deleted the postgres_patch_update branch July 8, 2021 23:26
@neelip neelip added this to the 0.18.0 milestone Jul 16, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants