-
Notifications
You must be signed in to change notification settings - Fork 201
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
feat(cli, sdk): resource usage report #4741
Conversation
Console preview environment is available at https://wing-console-pr-4741.fly.dev 🚀 Last Updated (UTC) 2023-10-31 15:49 |
BenchmarksComparison to Baseline ⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜
⬜ Within 1.5 standard deviations Benchmarks may vary outside of normal expectations, especially when running in GitHub Actions CI. Results
Last Updated (UTC) 2023-10-31 16:05 |
Signed-off-by: monada-bot[bot] <[email protected]>
Signed-off-by: monada-bot[bot] <[email protected]>
@@ -176,6 +176,33 @@ export abstract class Resource extends Construct implements IResource { | |||
|
|||
private readonly onLiftMap: Map<IInflightHost, Set<string>> = new Map(); | |||
|
|||
constructor(scope: Construct, id: string) { |
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.
Can we implement this without adding a dependency on std.Resource
? We have some plans to deprecate this class (see #3717)
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'll need to attach it to whatever class that replaces that- it has to be a parent class for all of the resources (even if it will be empty besides this constructor) should I add a comment?
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'll need to attach it to whatever class that replaces that
You can safely assume every preflight class extends constructs.Construct
.
Maybe it's possible to inject this logic in the App's new
function? see here:
wing/libs/wingsdk/src/core/app.ts
Lines 203 to 218 in cac0b1d
public new( | |
fqn: string, | |
ctor: any, | |
scope: Construct, | |
id: string, | |
...args: any[] | |
): any { | |
// delegate to "tryNew" first, which will allow derived classes to inject | |
const instance = this.tryNew(fqn, scope, id, ...args); | |
if (instance) { | |
return instance; | |
} | |
// no injection, so we'll just create a new instance | |
return new ctor(scope, id, ...args); | |
} |
Hi, This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days. |
Hi, This PR has not seen activity in 20 days. Therefore, we are marking the PR as stale for now. It will be closed after 7 days. |
Way too many conflicts. closing that one and opening #5243 instead |
Description
closes #4644
As a part of the matrix automation process, we need to figure out what are the methods and properties that are used in each test. I came out with the following approach:
_addOnLift
Please let me know what you think :)
TODO:
Checklist
pr/e2e-full
label if this feature requires end-to-end testingBy submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.