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.
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
EventGridv2 TypeSpec Api Preview #23204
EventGridv2 TypeSpec Api Preview #23204
Changes from 13 commits
68f887b
6554c10
eb98a20
c879030
6b989da
4a5f9cf
5511313
1a56614
c724eed
f91ee18
eb01ed4
69ccc41
0d2cb3a
c2d4c68
36d0e06
15dccec
13f6949
069bafb
5d21632
17fd291
663b348
fd1fd9d
02ea7de
17d65e2
3806bc1
799d10c
10e0e8c
09f9244
6c2b560
20958fd
16602df
911c47f
418e39a
e570388
0b24c89
a90210c
d1c21b5
4b1d4a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
While testing the service, what we see is that the only autg support is:
Authorisation: SharedAccessKey
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.
Confirmed with the team and you are right. I updated it
@useAuth(
ApiKeyAuth<ApiKeyLocation.header, "SharedAccessKey"> | OAuth2Auth<[{
type: OAuth2FlowType.implicit,
authorizationUrl: "https://login.microsoftonline.com/common/oauth2/v2.0/authorize",
scopes: ["https://eventgrid.azure.net/.default"],
}]>
)
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.
@ahamad-MS This is not correct. The header name is
Authorisation
, and the value isSharedAccessKey \<insert your key\>
. Your TypeSpec says that the header name isSharedAccessKey
, and that we can put the key as value, which is not what we saw.My question for the stewardship is wider though: is that an ok way to auth? The fact that we cannot describe this in TypeSpec today, is likely an indicator that it's not following Azure guidelines recommendation on auth.
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.
Should this be changed to
unknown
?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.
https://github.com/Azure/azure-rest-api-specs/pull/23539/files
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.
Here, and directly below, we prefer camelCase for property names and enum values.
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.
Because this is following the outside spec of CloudEvent, it requires these parameters to be lowercase
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.
We can't have the model and property name be the same thing. Can this be called something like LockTokenProperties?
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.
Also, possibly too late for the initial preview, but I really wonder whether it is worth having this model vs simply defining the lockToken as a string on each of the containing models. In my mind, the lock token is self sufficient and any supplementary information that might be added in the future may be context-dependent and can be added to the container model. Especially considering we don't even use this model for both input and output (only output).
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.
Why does
failedLockTokens
have a type ofFailedLockToken
butsucceededLockTokens
have typestring
? Either one of these is typed incorrectly or one of them is not an array of "LockTokens" as the name suggests.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.
FailedLockToken contains additional information - the errorCode and errorDescription. So the docs need updating to clarify that it is an array of FailedLockToken values.
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.
Updating the docs is certainly welcome, but I think some users will still find the naming confusing.
A "LockToken" is a string. A "succeededLockToken" is a LockToken (string) that succeeded. A "failedLockToken" is not a LockToken that failed -- it is a completely different type.
Rather than "failedLockToken", I think a better name would be "LockTokenFailure" -- since it describes the failure for a lock token. Is it too late to make a name change?
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.
It's not really a LockTokenFailure - it's a failure in one of the settlement operations (acknowledge or release), and the lock tokens that can be used to correlate the failures with the events that were received via the receive operations are included in the payload.
Maybe something like "failureInformation"?
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.
Why is this string array rather than LockToken array?
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 guess the LockToken type is meant for output that might evolve over time.
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.
Why not a collection action of a
Topic
resource?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.
From testing the server, we indeed receive a 200. We would have expected a 204, not a 200, given this returns no body.
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.
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.
Same as before:
From testing the server, we indeed receive a 200. We would have expected a 204, not a 200, given this returns no body.
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.
Why not a collection action over an
EventSubscription
resource?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.
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 wanted to name this as maxWaitTime - is there a way to do that given this is query param sent to the service?
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.
Is there some format we can apply so this generates as a TimeSpan,Duration,etc? In swagger, we would use format: date-time.
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 projected name can be used here for renaming
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.
Cool, is there an equivalent for
format: date-time
?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.
Type it as
duration
. We're still working on specifiying that the serialization format is an int, so let's keep int for now short termThere 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.
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.
What we see calling the service, is that nothing to receive returns an empty list with a 200, and if there are events this is a 201
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.
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.