Add service id to the error message to aid debugging #2483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kstich
requested changes
Dec 6, 2024
| TopDownIndex topDownIndex = TopDownIndex.of(model); | ||
| AwsTagIndex tagIndex = AwsTagIndex.of(model); | ||
| for (ServiceShape service : model.getServiceShapes()) { | ||
| for (ResourceShape resource : topDownIndex.getContainedResources(service)) { | ||
| boolean resourceLikelyTaggable = false; | ||
| if (resource.hasTrait(TaggableTrait.class)) { | ||
| if (resource.hasTrait(TaggableTrait.class) && service.getResources().contains(resource.getId())) { |
There was a problem hiding this comment.
This isn't right. It's only checking resource shapes bound directly to the service, meaning this won't run on nested resources (service -> parent -> child - child won't be checked.)
| TopDownIndex topDownIndex = TopDownIndex.of(model); | ||
| AwsTagIndex tagIndex = AwsTagIndex.of(model); | ||
| for (ServiceShape service : model.getServiceShapes()) { | ||
| for (ResourceShape resource : topDownIndex.getContainedResources(service)) { | ||
| boolean resourceLikelyTaggable = false; | ||
| if (resource.hasTrait(TaggableTrait.class)) { | ||
| if (resource.hasTrait(TaggableTrait.class) && service.getResources().contains(resource.getId())) { | ||
| events.addAll(validateResource(model, resource, service, tagIndex)); |
There was a problem hiding this comment.
The error messages in validateResource that are related to the service's tagging operations should include the ID of the service shape to aid in debugging.
sugmanue
force-pushed
the
sugmanue/exclude-non-bound-resources-from-validation
branch
from
0bf70f9 to
483d36b
Compare
sugmanue
changed the title
sugmanue
force-pushed
the
sugmanue/exclude-non-bound-resources-from-validation
branch
from
483d36b to
a496f79
Compare
sugmanue
commented
Dec 6, 2024
| @@ -87,7 +87,6 @@ private List<ValidationEvent> validateResource( | |||
| // list or tag keys input or output member | |||
| // 2. Tagging via APIs specified in the @taggable trait which are validated | |||
| // through the tag property, and must be resource instance operations | |||
| //Caution: avoid short circuiting behavior. | |||
There was a problem hiding this comment.
This comment is misleading, you only want to avoid short circuiting if you are expecting a side effect, that's not the case here.
sugmanue
commented
Dec 6, 2024
| if (!(isServiceWideTaggable || isInstanceOpTaggable)) { | ||
| events.add(error(resource, "Resource does not have tagging CRUD operations and is not compatible" | ||
| + " with service-wide tagging operations.")); | ||
| if (!isServiceWideTaggable && !isInstanceOpTaggable) { |
There was a problem hiding this comment.
The condition written this way expresses more clearly what we want.
sugmanue
commented
Dec 6, 2024
| Optional<String> property = resource.expectTrait(TaggableTrait.class).getProperty(); | ||
| if (property.isPresent()) { | ||
| if (operationId.isPresent()) { | ||
| if (operationId.isPresent()) { |
There was a problem hiding this comment.
The only way for this code to branch to true is for when the optional operationId is present. I moved it up to avoid unneeded work when that's not true.
kstich
approved these changes
Dec 6, 2024
sugmanue
deleted the
sugmanue/exclude-non-bound-resources-from-validation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
When a resource is in the closure of more than one service the
TaggableResourceerror won't say for which one it failed, e.g.,This change add the service to better understand where the issue is.
While there I also made minor changes to improve the code around.
Testing
Links
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.