Skip to content

Conversation

@alanenriqueo
Copy link
Member

@alanenriqueo alanenriqueo commented Oct 7, 2022


Contributing to the Azure SDK

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

What will be the example of using this method like? Can we possibly add a sample of this in the sample repo if possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, after this gets merged, I'll add a sample.

Copy link
Member

Choose a reason for hiding this comment

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

The word Execute looks a bit redundant, can we rename this operation to simply GetCachedServerName?

Copy link
Member Author

@alanenriqueo alanenriqueo Oct 12, 2022

Choose a reason for hiding this comment

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

Done, I renamed it to GetPostgreSqlFlexibleServerCachedServerName to have consistency with other operations.

Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. I saw the new version is 1.1.0-beta1, so it is OK. But you only change the sequence of the parameter, which should be avoided.

Copy link
Member

Choose a reason for hiding this comment

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

@alanenriqueo I just realized that there is already an official release 1.0.0. So this is a breaking change which needs to be resolved.

Here are 2 options:

Copy link
Member

Choose a reason for hiding this comment

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

Changing order of properties is no longer a brekaing change in track 2 SDKs

Copy link
Member

Choose a reason for hiding this comment

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

From the SDK API perspective, this isn't a good resource name. Why do we have two consecutive word Server? Can we omit one of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@alanenriqueo alanenriqueo force-pushed the fspg-flexibleserver-2022-03-privatepreview branch from 639f00c to 4ec2526 Compare October 12, 2022 04:27
@alanenriqueo alanenriqueo force-pushed the fspg-flexibleserver-2022-03-privatepreview branch from 4ec2526 to 42dbf73 Compare October 12, 2022 05:16
@alanenriqueo alanenriqueo marked this pull request as draft October 12, 2022 19:05
@alanenriqueo alanenriqueo marked this pull request as ready for review October 12, 2022 19:07
@alanenriqueo alanenriqueo marked this pull request as draft October 19, 2022 21:17
@ArthurMa1978 ArthurMa1978 added Mgmt This issue is related to a management package. PostgreSQL labels Oct 24, 2022
batch:
- tag: package-2020-01-01
- tag: package-flexibleserver-2021-06
- tag: flexibleserver-2022-03-privatepreview
Copy link
Member

Choose a reason for hiding this comment

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

The Azure/azure-rest-api-specs#20212 was merged 20 mins ago, do you need to update the sdk accordingly?

# Release History

## 1.1.0-beta.1 (Unreleased)
## 1.1.0-beta.1 (2022-10-11)
Copy link
Member

Choose a reason for hiding this comment

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

Please update this to a new release day as the swagger has just been merged 20 mins ago

Copy link
Member

Choose a reason for hiding this comment

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

And please also flip the pr from draft to Ready for review

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Dec 23, 2022
@ghost
Copy link

ghost commented Dec 23, 2022

Hi @alanenriqueo. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost closed this Dec 30, 2022
@ghost
Copy link

ghost commented Dec 30, 2022

Hi @alanenriqueo. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mgmt This issue is related to a management package. no-recent-activity There has been no recent activity on this issue. PostgreSQL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants