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

fix ARM scenario names #714

Merged
merged 16 commits into from
Oct 17, 2024
Merged

Conversation

XiaofeiCao
Copy link
Contributor

@XiaofeiCao XiaofeiCao commented Aug 29, 2024

Related comment: #701 (comment)

Shouldn't affect existing generated SDK code. Verified for Java, codegen PR: microsoft/typespec#4717
May affect existing coverage data in dashboard, though should be auto-updated after each SDK's generator CI runs.

Breaking changes:

1. Renamed namespace `Azure.ResourceManager.Models.Resources` to `Azure.ResourceManager.Resources`.
2. Renamed namespace `Azure.ResourceManager.Models.CommonTypes.ManagedIdentity` to `Azure.ResourceManager.CommonProperties`.
3. Renamed folder `common-types/managed-identity` to `common-properties`.
4. Renamed `ManagedIdentityTrackedResources` interface in `common-properties` to `ManagedIdentity`.

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

Copy link

changeset-bot bot commented Aug 29, 2024

🦋 Changeset detected

Latest commit: 9ee03d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@azure-tools/cadl-ranch-specs Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@XiaofeiCao
Copy link
Contributor Author

@iscai-msft I've renamed the namespace according to #703 (comment)

I removed the extra s, e.g. Resources->Resource, guess it's better to be shown in dashboard.

Let me know your thoughts.

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

thank you for all of your work @XiaofeiCao, I think this definitely is almost there. @timotheeguerin if you could take a look with your ARM knowledge, that would be great. Thank you so much!

packages/cadl-ranch-specs/cadl-ranch-summary.md Outdated Show resolved Hide resolved
packages/cadl-ranch-specs/cadl-ranch-summary.md Outdated Show resolved Hide resolved
@iscai-msft
Copy link
Contributor

@XiaofeiCao is this PR ready to re-review?

@XiaofeiCao
Copy link
Contributor Author

@XiaofeiCao is this PR ready to re-review?

Yes, please! Would like a re-review for this PR, since it's required for future ARM test cases.

I added breaking change note in changeset, let me know if it looks good.

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

Thanks @XiaofeiCao this looks great! Can you make sure @timotheeguerin approves first, and that folks in shanghai are aware of the breaking change? After that is done, should be good to merge. Thanks again for all of your changes and all the work you've put into this, it looks great

packages/cadl-ranch-specs/cadl-ranch-summary.md Outdated Show resolved Hide resolved
@XiaofeiCao
Copy link
Contributor Author

XiaofeiCao commented Sep 24, 2024

Thanks @XiaofeiCao this looks great! Can you make sure @timotheeguerin approves first, and that folks in shanghai are aware of the breaking change? After that is done, should be good to merge. Thanks again for all of your changes and all the work you've put into this, it looks great

Yes, shanghai folks are informed of this breaking change. Thanks a lot for your suggestions!

@XiaofeiCao XiaofeiCao mentioned this pull request Oct 14, 2024
4 tasks
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft left a comment

Choose a reason for hiding this comment

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

Recommend that you tested this in Java (as it seems involve some mockapi change as well).

@XiaofeiCao
Copy link
Contributor Author

Recommend that you tested this in Java (as it seems involve some mockapi change as well).

Yeah, regened and tested: microsoft/typespec#4717

@XiaofeiCao XiaofeiCao merged commit 538eb76 into Azure:main Oct 17, 2024
9 checks passed
github-merge-queue bot pushed a commit to Azure/typespec-azure that referenced this pull request Oct 25, 2024
…1712)

- origin: Azure/cadl-ranch#714
- Fixed singleton mockapi.ts. Location is not updatable. Tested locally.
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