SYSENG-1822: panic when faking ResourceWithTag #419
Conversation
|
Code Climate has analyzed commit 9dffea8 and detected 0 issues on this pull request. View more on Code Climate. |
DrPsychick
added
integration_tests
89Q12
added
integration_tests
DrPsychick
added
integration_tests
| @@ -91,7 +90,7 @@ func (rwt ResourceWithTag) EndpointURL(ctx context.Context) (*url.URL, error) { | |||
| return nil, fmt.Errorf("%w: ResourceWithTag only support Create and Destroy operations", api.ErrOperationNotSupported) | |||
| } | |||
|
|
|||
| return url.Parse(fmt.Sprintf("/api/core/v1/resource.json/%v/tags/%v", rwt.Identifier, rwt.Tag)) | |||
| return url.Parse(fmt.Sprintf("/api/core/v1/resource.json/%v/tags/%v", rwt.ResourceIdentifier, rwt.Tag)) | |||
There was a problem hiding this comment.
The ticket you've created says:
When using the mock, this causes a resource object to be replaced in the mock data with the ResourceWithTag object because they have the same identifier.
It's clear that this is a problem inside the implementation of the mock, not of the actual API. There are two solutions to this problem:
- Either provide a way that sets the
ResourceIdentifierin a backwards compatible way for existing users or - fix the wrong behavior in the mock itself.
My preference would be option 2, but I'm fine with option 1 as well.
There was a problem hiding this comment.
We agreed on adjusting the code so it is backwards compatible, using Identifier when no ResourceIdentifier is available.
There was a problem hiding this comment.
Thanks for that feedback, I additionally found an issue with the tests and fixed it as well.
DrPsychick
force-pushed
the
SYSENG-1822-fix-ResourceWithTag
branch
from
9fa980b
to
632101a
Compare
DrPsychick
changed the title
ResourceWithTag expects the identifier of a resource that is to be tagged. The tag itself has its own identifier which is returned in the response from the Engine.
Increase timeout for eventually to make lbaas tests pass. Move DeferCleanup into BeforeAll for vlan e2e test.
Support using ResourceWithTag as before with 'Identifier.' Cover it with tests. Cover ListTags with a simple test.
Remove ListTags test as it is unrelated to this change.
Reverts commit e937d73. The source of the issue was faking ResourceWithTag instead of setting tags or tagging a resource.
Co-authored-by: Jasmin <[email protected]>
DrPsychick
force-pushed
the
SYSENG-1822-fix-ResourceWithTag
branch
from
459fcf8
to
9dffea8
Compare
Description
Update: after digging deeper, it turns out that only the use of
ResourceWithTagdirectly with the mock was an issue. To use tags in mock data, createResourcewhich has tags instead.ResourceWithTag expects the identifier of a resource that is to be tagged. The tag itself has its own identifier which is returned in the response from the Engine.This fixes the issue that aResourceWithTagusing the same identifier as a Resource will overwrite each other in mock data.Additionally, this fixes two small issues with integration tests not passing.
Checklist
Unreleasedsection in CHANGELOG.md, if user facing changeReferences
Community Note