feat(cli): support for notices #18936
Conversation
|
|
| } | ||
|
|
||
| interface CachedNotices { | ||
| expiration: number, |
There was a problem hiding this comment.
I still find this a little weird. Why wouldn't we use the mtime of the cached notices file?
There was a problem hiding this comment.
You're probably right. But I think I've got a bit of a tunnel vision here: the code for retrieving the contents (including some form of timestamp) and the code for comparison/retrieval should be kept separate. So we need some data structure to send data from one to the other, which means this interface would still exist. The only difference is that the expiration time would be a little more hidden.
Now tell me how I am wrong :-)
There was a problem hiding this comment.
Ahhh, you're not wrong. I guess it doesn't matter. I'm having a hard time thinking up cases in which they produce different results.
|
Can you add a screenshot with an example? |
packages/aws-cdk/lib/cli.ts
Outdated
| @@ -227,6 +230,8 @@ if (!process.stdout.isTTY) { | |||
| } | |||
|
|
|||
| async function initCommandLine() { | |||
| void refreshNotices().then(_ => debug('Notices refreshed')); | |||
There was a problem hiding this comment.
Probably also needs a catch.
| } | ||
|
|
||
| case 'notices': | ||
| // This is a valid command, but we're postponing its execution |
There was a problem hiding this comment.
I would have put the command here, but I guess you're not doing that on purpose.
There was a problem hiding this comment.
Yes, if we put the command here, it will show the notices again at the end, unless we have some way of signaling that notices were already shown and there is no need to show it again. I don't like either approach, but this one seems a bit cleaner.
packages/aws-cdk/lib/cli.ts
Outdated
| } | ||
| } finally { | ||
| await version.displayVersionMessage(); | ||
| } | ||
|
|
||
| async function main(command: string, args: any): Promise<number | string | {} | void> { |
There was a problem hiding this comment.
Signature should be changed to Promise<number | undefined>.
packages/aws-cdk/lib/notices.ts
Outdated
| } | ||
|
|
||
| export async function refreshNotices() { | ||
| const dataSource = dataSourceReference(true); |
There was a problem hiding this comment.
I don't think this necessarily MUST ignore the cache... just that IF we're going to refresh, we'd better do it early so that we don't add latency later. It's perfectly fine not to refresh here if the data is not stale yet.
| } | ||
|
|
||
| interface CachedNotices { | ||
| expiration: number, |
There was a problem hiding this comment.
Ahhh, you're not wrong. I guess it doesn't matter. I'm having a hard time thinking up cases in which they produce different results.
Type of main updated; Refresh doesn't force a cache update
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
otaviomacedo
removed
the
pr/do-not-merge
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Features
cdkcommandcdk acknowledgewill acknowledge an issue by id, scoped to individual cdk appscdk noticesalways returns relevant notices'notices' = falsewill hide notices always--no-noticesoption--fail-on-noticesoption -- this will be left for a separate PRExample:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license