-
Notifications
You must be signed in to change notification settings - Fork 50
Invalidate example cache keys more aggressively #3214
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3214 +/- ##
===========================================
- Coverage 68.73% 46.93% -21.80%
===========================================
Files 337 316 -21
Lines 44184 42870 -1314
===========================================
- Hits 30368 20120 -10248
- Misses 12088 20910 +8822
- Partials 1728 1840 +112 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
This looks so much more clean in addition to changing on more conditions.
Thank you!
Could we please change the variable names? I've based my suggestions on https://google.github.io/styleguide/go/decisions#variable-names, which I think is a pretty excellent read.
While that guide isn't canonical, it is what's linked from the much less getailed Go Style Wiki so I'd consider it a solid guideline.
| "github.com/pulumi/pulumi-terraform-bridge/v3/pf", | ||
| v := map[string]string{} | ||
|
|
||
| bi, ok := debug.ReadBuildInfo() |
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.
| bi, ok := debug.ReadBuildInfo() | |
| buildInfo, ok := debug.ReadBuildInfo() |
This is honestly more because "bi" reminds me of the prefix from "binary" and could just be a personal preference.
| "github.com/pulumi/pulumi/pkg/v3", | ||
| "github.com/pulumi/pulumi-terraform-bridge/v3", | ||
| "github.com/pulumi/pulumi-terraform-bridge/v3/pf", | ||
| v := map[string]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.
Because we're using it across the entire method, can we give v a descriptive name?
| v := map[string]string{} | |
| versions := map[string]string{} |
| "github.com/pulumi/pulumi-terraform-bridge/v3": | ||
|
|
||
| v[mod.Path] = mod.Version | ||
| 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.
should we return versions directly here?
| PulumiVersion string `json:"pulumiVersion"` | ||
| SoftwareVersions map[string]string `json:"softwareVersions"` | ||
| BuildFileHashes map[string]string `json:"buildFileHashes"` | ||
| Plugins map[string]map[string]string `json:"plugins"` |
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 noticed that the way we calculate the plugins is via pulumi plugin ls which apparently does not take into account PATH manipulation.
The way we currently do it (using the .pulumi local folder) and the way we plan to do it (with mise) work by putting the plugins on the PATH. This might be something that we have to fix with pulumi plugin ls
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.
Nice, thanks for improving this! The cache is gnarly but important for us. Somehow uncached AWS builds were still in the 30 min range for me last time I worked on this.
The cache key we use for examples is currently derived from a hash of the HCL and the target language. This causes us to incorrectly skip generating examples when the converter is upgraded, when the provider's version changes (#2323), or when the provider's flags are changed (pulumi/ci-mgmt#1025).
This changes updates our key to derive from a couple more inputs:
Fixes pulumi/ci-mgmt#1025.