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.

Sign up for GitHub

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 Lint error #13765

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Fix Lint error #13765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dedd1a1
Add missing default error response
jiacheng-L Apr 1, 2021
61e765e
Update the integer format to fix Lint error
jiacheng-L Apr 1, 2021
0872f5a
Add securityDefinitions
jiacheng-L Apr 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...ation/keyvault/resource-manager/Microsoft.KeyVault/preview/2021-04-01-preview/common.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,16 @@
"modelAsString": true
}
}
},
"securityDefinitions": {
"azure_auth": {
"type": "oauth2",
"authorizationUrl": "https://login.microsoftonline.com/common/oauth2/authorize",
"flow": "implicit",
"description": "Azure Active Directory OAuth2 Flow",
"scopes": {
"user_impersonation": "impersonate your user account"
}
}
}
}
14 changes: 10 additions & 4 deletions ...tion/keyvault/resource-manager/Microsoft.KeyVault/preview/2021-04-01-preview/secrets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,12 @@
"schema": {
"$ref": "#/definitions/SecretListResult"
}
},
"default": {
"description": "Error response describing why the operation failed.",
"schema": {
"$ref": "common.json#/definitions/CloudError"
}
}
},
"x-ms-examples": {
Expand Down Expand Up @@ -300,24 +306,24 @@
"nbf": {
"x-ms-client-name": "NotBefore",
"type": "integer",
"format": "unixtime",
"format": "int64",
Copy link
Member

Choose a reason for hiding this comment

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

Is this done only for consistently with other control-plane definitions for KV? In practice, it's a pain: there are no ARM template functions to calculate or format this value, which means it has to be done outside the ARM template and passed in if people want to reuse the ARM template, which is one of ARM templates' goals: reusability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @heaths , here is the only place that we use unixtime for integer in control-plane. Now the swagger do this job, to validate the integer format. They only allow int32 or int64: https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/openapi-authoring-automated-guidelines.md#R4013

Copy link
Member

@heaths heaths Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, an integer type needs a format. The point of my question, though, is whether this should be an integer at all - or a string using the datetime format.

ARM templates are meant to be reusable. There are ARM functions that, for example, can calculate datetimes and format them to standard ISO 8601 formats. So if you wanted to use a template written a year ago to provisioning a Key Vault and secret (or key) using an expiration date for reasonable now (say, today + 90 days), these integers do not work. They have to be passed in as parameters (and, hence, calculated outside the ARM template by some other scripts) or changed in the file itself - both of which make reusability of the ARM template unwieldy.

I recently came up against this trying to help customers on Stack Overflow. These offsets as integers since the 1970-01-01 epoch are not useful.

/cc @jlichwa

Copy link
Contributor

Choose a reason for hiding this comment

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

@heaths I agree that using Unix Time in ARM templates is really bad experience (ARM template don't have function to convert datetime to unix time. and I went over many Stack Overflow issues complaining about it)
Our service (REST API for both planes) requires unix time today for all dates and that is what has been used, so I don't think we can change it without updating service, which will be breaking change.

I think we should create a feature request to replace all unix time with date time or anything else which is more dev friendly, but due to this being breaking change across many APIs, it is not an easy effort.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. That's what I was wondering: if this is a change to make the swagger correct, or if there's opportunity to fix the problem now.

Bicep is considering (or at least there's "tons" of users requesting it) user-declared functions, but I wonder if requesting more date functions for ARM templates is feasible.

Copy link
Contributor Author

@jiacheng-L jiacheng-L Apr 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This change will be merged to PR: #13585, which is the new API version 2021-04-01-preview for control plane. That PR reported the integer format that must be fixed. As it is a new API version and swagger block this, I think it is OK to fix it now.

"description": "Not before date in seconds since 1970-01-01T00:00:00Z."
},
"exp": {
"x-ms-client-name": "Expires",
"type": "integer",
"format": "unixtime",
"format": "int64",
"description": "Expiry date in seconds since 1970-01-01T00:00:00Z."
},
"created": {
"type": "integer",
"format": "unixtime",
"format": "int64",
"readOnly": true,
"description": "Creation time in seconds since 1970-01-01T00:00:00Z."
},
"updated": {
"type": "integer",
"format": "unixtime",
"format": "int64",
"readOnly": true,
"description": "Last updated time in seconds since 1970-01-01T00:00:00Z."
}
Expand Down