-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix: context provider's ignoreErrorOnMissingContext
parameter is misleading
#33875
base: main
Are you sure you want to change the base?
Conversation
ead00e7
to
ade07ed
Compare
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 review is outdated)
ade07ed
to
cc112a9
Compare
…sleading In `ContextProvider.getValue()`, `ignoreErrorOnMissingContext` is a request to the CLI's context provider to not fail the lookup, but return the dummy value instead. The operation doesn't have anything to do with missing context, and missing context isn't an error. Deprecate that parameter and add `ignoreFailedLookup` instead. Also explain the `dummyValue` field and this one a bit better.
cc112a9
to
443251d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33875 +/- ##
==========================================
- Coverage 82.38% 82.36% -0.03%
==========================================
Files 120 120
Lines 6938 6940 +2
Branches 1170 1171 +1
==========================================
Hits 5716 5716
- Misses 1119 1120 +1
- Partials 103 104 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Co-authored-by: Kenta Goto (k.goto) <24818752+go-to-k@users.noreply.github.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
ignoreFailedLookup
vs.
The dummy value should not be returned for all SDK lookup failures. For example, "no network" or "no credentials" or "malformed query" should not lead to the dummy value being returned. Only the case of "no such resource" should.`
Why not call it ignoreResourceNotFound
(or similar) then?
And then because ignore
always implies you know what the standard behavior is, we might be better off calling it failOnResourceNotFound
(with a value defaulting to true
). Or even something like mustReturnResource
.
In
ContextProvider.getValue()
,ignoreErrorOnMissingContext
is a request to the CLI's context provider to not fail the lookup, but return the dummy value instead.The operation doesn't have anything to do with missing context, and missing context isn't an error. Deprecate that parameter and add
ignoreFailedLookup
instead.Also explain the
dummyValue
field and this one a bit better.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license