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.

Sign up for GitHub

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

Jump to bottom

Hosts context #2017

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Hosts context #2017

merged 10 commits into from
Nov 21, 2023

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Nov 20, 2023 •
edited

Description

Update hosts related code to a "hosts phoenix context".

  • The hearbeats goes into its own context.
  • saptune and sles subscriptions belong to the hosts context
  • the dependant value objects are moved to the hosts folder
  • legacy events are moved to the a legacy folder
  • mix typo in event -> HeartbeatSucceded -> HeartbeatSucceeded

How was this tested?

Tests were simple renamed and moved

@arbulu89 arbulu89 added enhancement New feature or request chore labels Nov 20, 2023
@arbulu89 arbulu89 marked this pull request as ready for review November 21, 2023 07:51
CDimonaco
CDimonaco approved these changes Nov 21, 2023
View reviewed changes
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.

LGTM

Good job, I'm reallu happy with that, just a sidenote comment, maybe we should put the ValueObject in the module name of value objects?

Elixir module system gives us the ability to decouple the folder from the module name, but idk if we have to specify that in this refactor, I think yes, but no strong opinions, wdyt?

rtorrero
rtorrero approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

LGTM

@arbulu89
Copy link
Contributor Author

LGTM

Good job, I'm reallu happy with that, just a sidenote comment, maybe we should put the ValueObject in the module name of value objects?

Elixir module system gives us the ability to decouple the folder from the module name, but idk if we have to specify that in this refactor, I think yes, but no strong opinions, wdyt?

Trento.Hosts.SaptuneNote to Trento.Hosts.ValueObjects.SaptuneNote, right?
Yes, let's do it. Otherwise, it looks like an exception, and we don't want to add exceptions without good reasons

@arbulu89 arbulu89 merged commit d2879f1 into main Nov 21, 2023
26 checks passed
@arbulu89 arbulu89 deleted the hosts-context branch November 21, 2023 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtorrero rtorrero rtorrero approved these changes

@CDimonaco CDimonaco CDimonaco approved these changes

@EMaksy EMaksy Awaiting requested review from EMaksy

Assignees
No one assigned
Labels
chore enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arbulu89 @rtorrero @CDimonaco