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

Reorganize directory structure for clusters context #2012

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Nov 17, 2023

Description

This PR applies the context-based reordering from ADR#10 for clusters

@rtorrero rtorrero force-pushed the clusters-context branch 2 times, most recently from 5a152bc to d174981 Compare November 22, 2023 08:54
@rtorrero rtorrero marked this pull request as ready for review November 22, 2023 10:49
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hey @rtorrero
2 things.

  • Try to put Clusters aliases before Hosts aliases, to have a correct alphabetical order
  • I think your formatter is broken somehow, as it is adding parenthesis to many field entries, which we shouldn't have

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hey @rtorrero
More things you can fix while the other PR is merged.
You need to adapt the test folders as well, as I only see that you change imported aliases.
The test folder must follow the restructure as well

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thank you @rtorrero
Some final changes. You don't need other review from me, but i would encourage to get other approval at least, as it is a sensitive chagne

test/support/factory.ex Outdated Show resolved Hide resolved
@@ -7,7 +7,11 @@ defmodule Trento.ClustersTest do

import Trento.Factory

alias Trento.{ClusterEnrichmentData, ClusterReadModel, Clusters}
alias Trento.Clusters
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this file to test/trento/clusters_test.exs

@@ -1,11 +1,11 @@
defmodule Trento.ClusterTest do
use Trento.AggregateCase, aggregate: Trento.Domain.Cluster, async: true
use Trento.AggregateCase, aggregate: Trento.Clusters.Cluster, async: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this file to test/trento/clusters/cluster_test.exs

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey besides @arbulu89 nothing more to add :)

@rtorrero rtorrero merged commit f70c925 into main Nov 27, 2023
26 checks passed
@rtorrero rtorrero deleted the clusters-context branch November 27, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants