-
Notifications
You must be signed in to change notification settings - Fork 37
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
add resource type scalar in typespec-azure-core
#1272
Open
ArcturusZhang
wants to merge
11
commits into
Azure:main
Choose a base branch
from
ArcturusZhang:add-resource-type
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+41
−7
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b3c0834
add resource type scalar and put it in some corresponding places
ArcturusZhang a95653d
add changelog
ArcturusZhang ac93669
update the gen sample to ensure it does not change
ArcturusZhang de619ce
rebuild
ArcturusZhang a3c466c
Merge branch 'main' into add-resource-type
ArcturusZhang 2b68cf3
Merge branch 'main' into add-resource-type
ArcturusZhang 3494df5
Merge branch 'main' into add-resource-type
ArcturusZhang 47aa996
Merge remote-tracking branch 'origin/main' into add-resource-type
ArcturusZhang 914fb65
Update .chronus/changes/add-resource-type-2024-6-31-14-6-51.md
ArcturusZhang 840aa34
Merge branch 'main' into add-resource-type
ArcturusZhang a1a3730
Merge branch 'main' into add-resource-type
ArcturusZhang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
--- | ||
changeKind: feature | ||
packages: | ||
- "@azure-tools/typespec-azure-core" | ||
- "@azure-tools/typespec-azure-resource-manager" | ||
--- | ||
|
||
Add `resourceType` scalar, and changed some properties to be resourceType instead of string |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was OOF the last 2 weeks, has there been agreement we needed this and this was the name we wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - I do not know. Do we need to bring up this topic in one of our typespec sync up meeting or your internal typespec sync up meeting?
In those meetings that I have participated in, this has not been a topic there.
Or maybe we could put this as an agenda in our next typespec sync up meeting on your wednesday night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah would be good, do you need this for this release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably need to differentiate this as we did with armResourceIdentifier - armResourceType.
There is definitely use for this in ARM, and since the type is needed to properly specify the allowedResourceType in armResourceIdentifier, it seems appropriate to put this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree the name should be
armResourceType
which makes more sense.The second part is that you suggest we add a generic argument for allowed resource type? like this?
I think this looks weird because this type is modeling the resource type of myself, if there is a limited list of resource types that this field is supporting, should we define a extensible enum based on resource type like this?
Well but I could see values for this because we could update the definition of
Resource
andTrackedResource
to let them accept another argument of resource type like this:so that we finally have explicitly defined resource type on resources, currently arm resources could have resource types but they are implicitly - we can infer them from the uri paths we built for resource related operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think using a union here instead to be more precise makes more sense instead of the
armREsourceIdentifier
which does that for a subset of the value.and
+1
on the name beingarmResourceType