-
Notifications
You must be signed in to change notification settings - Fork 43
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
Script converter metrics comparison #1349
Conversation
Diff for pulumi-azuread with merge commit 5d0172a |
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.
Looks good. Can we add a comment explaining that the script is temporary or put it in unstable/scripts/...
?
Tests look like they are failing because of pulumi/ci-mgmt#537 (fixed by #1350).
#1280 is the context. Yes will do. |
Diff for pulumi-azuread with merge commit 54e2ecb |
415c4c3
to
da7d247
Compare
Diff for pulumi-random with merge commit 1110332 |
Diff for pulumi-azuread with merge commit 1110332 |
Diff for pulumi-random with merge commit 0fd9570 |
Diff for pulumi-azuread with merge commit 0fd9570 |
Codecov Report
@@ Coverage Diff @@
## master #1349 +/- ##
==========================================
- Coverage 61.87% 57.83% -4.04%
==========================================
Files 182 282 +100
Lines 32708 39468 +6760
==========================================
+ Hits 20238 22827 +2589
- Misses 11332 15307 +3975
- Partials 1138 1334 +196 see 103 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. 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.
It looks like this script needs to be run in the directory of the provider that it is testing. That would be an excellent thing to put in a comment.
unstable/scripts/exconverter/main.go
Outdated
return s | ||
} | ||
|
||
func tfgen(convert int) { |
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'm getting strong C vibes here. Why isn't convert a bool
?
var entry flattenedExample | ||
err := dec.Decode(&entry) | ||
if err != nil || entry.ExampleName == "" { | ||
break |
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 feels like this should be a continue
. Why are we stopping on the first error?
Why don't we report the error?
Diff for pulumi-random with merge commit ea27b23 |
Diff for pulumi-azuread with merge commit ea27b23 |
Diff for pulumi-random with merge commit d1f0feb |
Diff for pulumi-azuread with merge commit d1f0feb |
Diff for pulumi-random with merge commit 7872a21 |
Diff for pulumi-azuread with merge commit 7872a21 |
I'd like to add this script temporarily to keep it checked in while we're rolling out the new PULUMI_EXPERIMENTAL converter. It will help computing the metrics difference between baseline/experimental, something I can attach to PRs during rollout.