-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add note on app.ContainerApp API versions and KV secrets #3284
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
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 helpful.
Out of interest, have we tried bumping the default version just to see if it's compatible?
I ran this, indicating that it's not.
|
Great, thanks for confirming! |
b3beb48
to
af586ec
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3284 +/- ##
=======================================
Coverage 55.33% 55.33%
=======================================
Files 66 66
Lines 9930 9930
=======================================
Hits 5495 5495
Misses 4001 4001
Partials 434 434 ☔ View full report in Codecov by Sentry. |
If we find that the default version is broken in some meaningful way, I would support us bumping it up to a better version even with minor breaking changes. It's a trade-off, but if we can take a closer look at what happened with Side note: please add some description to the PR. I know it feels like busywork, but I'd really want to encourage us to keep a high bar on PR text quality. |
The difference in the spec is that So, seems like changing the API version would require everyone to update their code, not only changing the property name but also looking up the correct value. On the positive side, it's limited to that one property. |
Addendum: v3 of the provider is also not too far away. Overall, we've decided to leave the API version as-is. |
Prompted by #3243. When creating a ContainerApp referencing a Key Vault secret via URL, Azure can at least sometimes respond with "invalid: value or keyVaultUrl and identity should be provided". This is a bug in the particular API version we use as default.
This PR adds a warning and a pointer later API versions to the registry docs.