Skip to content
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: remove unnamed consumed relation #709

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llorentelemmc
Copy link
Contributor

Issue #707

Simplified doRemoveResourceRelationForAllReleases() as it should not be necessary to identify a related resource as consumed or provided before deletion.

@llorentelemmc llorentelemmc changed the title fix: remove unnamed relation fix: remove unnamed consumed relation Oct 26, 2023
@llorentelemmc llorentelemmc requested a review from mburri October 26, 2023 14:38
@yvespp
Copy link
Member

yvespp commented Oct 31, 2023

The loop over all the relations is needed actually. Resources can have multipile release (Release -> new in the UI).
If a resource with multiple releases is added to an other resource, then a relation is created between all resource releases.
So it will look like this:

r1_rl1 -> r2_rl1
       -> r2_rl2

So when a relation is deleted in the UI, all relations from r1_rl1 to r2 have to be deleted.

@mburri
Copy link
Member

mburri commented Nov 1, 2023

Thanks for your input. I already suspected that we probably missed something.

What I also noticed: If you add a relation with multiple releases as a consumed resource and set the relation name - then the behavior is different: in this case only the currently selected relation is removed. Now we have the problem that removing unnamed resources does not work in our setup with wildfly.

I also noticed that there is zero test coverage and the code is quite hard to reason about. This means that the use cases (removing a relation from a resource) with all its special cases (resources with multiple releases, resourcetypes etc) are hard to understand and not documented anywhere.

I think it would be valuable to improve this situation:

  • Document the different use cases for removing relationships from resources.
  • Add tests to cover `ResourceRelationService#removeRelation(AbstractResourceRelationEntity relation)
  • Code refactorings to improve code readability

Since issue #707 was not part of the current milestone, I'd recommend marking this PR as a draft and moving it to a later release.

@mburri mburri marked this pull request as draft November 1, 2023 09:17
@yvespp
Copy link
Member

yvespp commented Nov 6, 2023

Refactor, tests & docs is always good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing relation of a consumed resource that has no relation name is not possible
3 participants