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

Instance deregistration endpoint #1730

Merged
merged 8 commits into from
Aug 24, 2023
Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Aug 22, 2023

Description

Add application/database instance deregistration (deletion) endpoints.

It adds the /api/v1/sap_systems/#{sap_system_id}/hosts/#{host_id}/instances/#{instance_number} (and the equivalent database endpoint). We need the 3 primary keys to know which instance to deregister.
Maybe there is some better way using params or similar, instead of having all the fields required in the field.

Besides that, it includes a basic domain logic to forbid the deletion of instances that are not absent.

The usage of the absent field logic in the endpoint is in the use cases code. This is because I didn't want to add this logic in the domain logic, as this would impact the whole deregistration processes (when the instances are deregistered on a host deregistration request for example). This way, this condition is only used when it these specific delete endpoints are used.

PD: The PR adds the fields and the migrations in the read models the way I would expect to be done in the side effects part

@nelsonkopliku @dottorblaster just putting you here if you want to have a look simply on the created routes. Maybe you have some suggestion in that matter

@arbulu89 arbulu89 force-pushed the instance-deregistration-endpoint branch 3 times, most recently from 76e2526 to e0ca40c Compare August 23, 2023 13:48
@arbulu89 arbulu89 force-pushed the instance-deregistration-endpoint branch from e0ca40c to 101b491 Compare August 23, 2023 13:55
@arbulu89 arbulu89 marked this pull request as ready for review August 23, 2023 14:32
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I al ok with the added routes.
Probably you already considered adding a /deregister suffix or similar and ditched it. I would not be against it.

I cannot but wonder about the relation of this PR with jamie's #1731 and the absent-deregister relation.
Are they two different things? As in marking something absent first and then eventually deregistering it? Am I getting it right?

Finally: the logic about being able to deregister or not an absent instance really looks like domain logic of the SAP system aggregate. I have not been in the loop, so I might be missing something.

Besides this just some typo fix suggestions.

@arbulu89
Copy link
Contributor Author

I cannot but wonder about the relation of this PR with jamie's #1731 and the absent-deregister relation. Are they two different things? As in marking something absent first and then eventually deregistering it? Am I getting it right?

Yes, they are totally related. This PR enables the user interaction to delete/deregister the instances from the web clicking a button, once they were marked as absent

Finally: the logic about being able to deregister or not an absent instance really looks like domain logic of the SAP system aggregate. I have not been in the loop, so I might be missing something.

I thought the same initially, but it totally affects the already existing deregistration logic. If we enforce this rule on the domain, we would need to do major changes in other parts. We would need to set all the instances belonging a host to absent if the host is deregisterable, which is not trivial at all.

I think all of this will have a follow up epic, where we need to start thinking of the state of the registered resources in hosts which are not reporting

Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job, lgtm!

@CDimonaco
Copy link
Member

I thought the same initially, but it totally affects the already existing deregistration logic. If we enforce this rule on the domain, we would need to do major changes in other parts. We would need to set all the instances belonging a host to absent if the host is deregisterable, which is not trivial at all.

Completely agree on that, for the moment we should keep that logic inside the usecase, but we can have a tech debt later to address this logic in the domain

@nelsonkopliku
Copy link
Member

@arbulu89 @CDimonaco ok I see the complexity of putting that logic where it belongs. So I ma fine with this transition.
Let's maybe just keep track of it.

@arbulu89 arbulu89 force-pushed the instance-deregistration-endpoint branch from 101b491 to 8d68cd3 Compare August 24, 2023 11:42
@arbulu89 arbulu89 added the enhancement New feature or request label Aug 24, 2023
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for paging me but I think you did a wonderful job 💪

@arbulu89 arbulu89 merged commit 0c49874 into main Aug 24, 2023
@arbulu89 arbulu89 deleted the instance-deregistration-endpoint branch August 24, 2023 15:01
EMaksy pushed a commit that referenced this pull request Aug 28, 2023
* Implemente absent instances deregistration domain logic

* Add deregistration use case functions

* Add the controller deletion endpoint

* Move asbsent usage logic to use case

* Update instances read models and add migrations

* Rename to absent_at

* Improve test texts using present term

* Use with clause to relay on fallback controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants