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

release for netapp mgmt #8451

Merged
merged 2 commits into from
May 19, 2020
Merged

release for netapp mgmt #8451

merged 2 commits into from
May 19, 2020

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Apr 21, 2020

No description provided.

/**
* List of Mount Targets
*/
export interface MountTargetList {
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is expected.Azure/azure-rest-api-specs@adf5808#diff-4f53ea72e2650e22c8b0ee2770c55654L2432 they remove the definition in their previous swagger PR

@qiaozha qiaozha merged commit 13665a4 into master May 19, 2020
@qiaozha qiaozha deleted the release-netapp-for-mgmt branch May 19, 2020 06:53
@ramya-rao-a
Copy link
Contributor

@qiaozha, @dw511214992, @RodgeFu, For future reference, I don't believe this change needed a major version update.

The net changes here were

  • Adding properties to model & mapper MountTarget
  • Removing model & interface MountTargetList

In a previous PR #8182 all public usage of MountTargetList was removed. Therefore, the removal of model & interface MountTargetList does not affect the end user. This PR needed only a minor or patch version update

@ramya-rao-a
Copy link
Contributor

Also, looks like this PR was created for the Azure/azure-rest-api-specs#9078. By the time the PR was approved, the swagger had changed due to a bug fix. See Azure/azure-rest-api-specs#9294. Due to this, we now have another PR #9198 where the major version of the package had to be updated again. With that, we come to 4 major version updates to this package in 2020.

Can we ensure we be a little careful with

  • checking if a major version update is indeed needed
  • checking if the swagger has changed from the time a PR is created to the time it is merged

Moving forward, it would also be helpful if the PR description has link to the swagger specification change that is resulting in the re-generation

Since version 9.0.0 of the package is buggy and the fix in the swagger was deemed as "urgent fix", can we consider deprecating the 9.0.0 version once 10.0.0 is released? Individual versions can be deprecated such that any current user would start getting warnings.

@qiaozha
Copy link
Member Author

qiaozha commented Jun 1, 2020

@qiaozha, @dw511214992, @RodgeFu, For future reference, I don't believe this change needed a major version update.

The net changes here were

  • Adding properties to model & mapper MountTarget
  • Removing model & interface MountTargetList

In a previous PR #8182 all public usage of MountTargetList was removed. Therefore, the removal of model & interface MountTargetList does not affect the end user. This PR needed only a minor or patch version update

Just wondering what if the user specifically import the model definition like the one in this issue? #7468 (comment)

@ramya-rao-a
Copy link
Contributor

That is a good point. In that case, I would have recommended to keep the interface lying around and do a minor version update for the changes to MountTarget. The next time there is a major version worthy change in the swagger, we could do the major version update in the sdk.

Basically, I am looking through the lens of the user. Everytime we do a major version update, the imrpovements in that and all future PRs are lost to users of current version. So, doing some checks on our side might be beneficial

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.

4 participants