search

HackerExperience / Helix

GNU Affero General Public License v3.0
53 stars 10 forks source link

Hardware refactor #338

Closed renatomassaro closed 6 years ago

renatomassaro commented 6 years ago

This will be a large PR. Here we'll refactor the Server and Hardware system, merging both into one.

I'll use a similar implementation to what was done with TOP, so each hardware component (CPU, RAM, HDD, NIC, USB); each motherboard (MoboDesktop, MoboMobile) and each Server (Desktop, Mobile) have a declarative interface in which they define how each one is supposed to behave.

Closes #233.

TODO

Incidental

This change is Reviewable

renatomassaro commented 6 years ago

Tests are now 33% faster (reduction from 30s to 20s)!

My guess: ServerFlow is used A LOT to generate test elements, and the previous flow issued several one-off calls to the DB, while here we batch them in some cases. Furthermore, we've removed the need to cache components, motherboard and its underlying resources, so we no longer make several calls to ETS and the database.

sourcelevel-bot[bot] commented 6 years ago

Ebert has finished reviewing this Pull Request and has found:

You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/338.

renatomassaro commented 6 years ago

Reviewed 85 of 159 files at r1, 45 of 65 files at r2, 6 of 7 files at r3. Review status: 135 of 170 files reviewed at latest revision, 34 unresolved discussions.

lib/account/action/flow/account.ex, line 16 at r3 (raw file):

  @spec setup_account(Account.t, Event.relay) ::
    {:ok, %{entity: Entity.t, server: Server.t}}
    | {:error, :internal}

This seems wrong

lib/account/event/handler/account.ex, line 6 at r3 (raw file):

  alias Helix.Account.Event.Account.Created, as: AccountCreatedEvent

  def account_created(event = %AccountCreatedEvent{}),

Docme

lib/cache/internal/cache.ex, line 46 at r3 (raw file):

    {:server, :nips} => {:server, :by_server, :networks},
    {:server, :storages} => {:server, :by_server, :storages},
    {:server, :resources} => {:server, :by_server, :resources},

:+1:

lib/cache/model/server_cache.ex, line 43 at r3 (raw file):

  end

  # def new(sid) do

remove comments

lib/cache/query/cache.ex, line 58 at r3 (raw file):

    do: CacheInternal.lookup({:server, :storages}, {server_to_id(server)})

  raisable {:from_server_get_resources, 1}

:tada:

lib/hell/hell/ecto_macros.ex, line 19 at r3 (raw file):

        unquote(block)
      end

Extra line

lib/hell/hell/macros_utils.ex, line 31 at r3 (raw file):

  end

  def atomize_module_name({_a, _s, [t]}),

docme

lib/hell/hell/macros_utils.ex, line 40 at r3 (raw file):

  end

  def get_parent_module(module) do

docme

lib/hell/hell/utils.ex, line 43 at r3 (raw file):

    do: String.to_atom(a <> b)

  def upcase_atom(a) when is_atom(a) do

docme

lib/hell/hell/utils.ex, line 50 at r3 (raw file):

  end

  def downcase_atom(a) when is_atom(a) do

docme

lib/hell/hell/utils.ex, line 97 at r3 (raw file):

    do: to_string(value)

  # TODO: Make Atom.upcase & Atom.downcase

I did make them. Remove

lib/network/action/network/connection.ex, line 21 at r3 (raw file):

  end

  def update_ip(nc = %Network.Connection{}, new_ip) do

spec & doc whole module

lib/network/internal/network/connection.ex, line 37 at r3 (raw file):

  """
  def create(network_id, ip, entity_id, nic_id \\ nil)
  def create(network, ip, entity, nic = %Component{type: :nic}),

This is ugly. See if it can be automatically casted by ecto

lib/server/README.md, line 1 at r3 (raw file):

# Hardware

Remove

lib/server/seed.ex, line 9 at r3 (raw file):

  alias Helix.Server.Repo

  def migrate do

docme

lib/server/component/specs/flow.ex, line 2 at r3 (raw file):

# credo:disable-for-this-file Credo.Check.Refactor.CyclomaticComplexity
defmodule Helix.Server.Component.Spec.Flow do

At least doc something

lib/server/component/specs/specable.ex, line 228 at r3 (raw file):

    @doc """
    Converts the mobo slots defined on the spec to a format used by Helix,
    which is :"<component_type>_<real_id>. So slot `0` for `hdd` is `:hdd_0`.

unbalanced quotes

lib/server/event/handler/account.ex, line 5 at r3 (raw file):

  alias Helix.Server.Action.Flow.Motherboard, as: MotherboardFlow

  def account_verified(event) do

docme

lib/server/internal/motherboard.ex, line 11 at r3 (raw file):

    Motherboard.t
    | nil
  def fetch(motherboard_id) do

mention that the return format is a reducer of all entries

lib/server/make/server.ex, line 23 at r3 (raw file):

    relay = nil

    # Setup mobo. TODO: Custom hardware for NPC

Create issue

lib/server/model/component.ex, line 43 at r3 (raw file):

  @type pluggable :: cpu | hdd | ram | nic

  @type type :: :cpu | :hdd | :ram | :nic | :mobo

This should be inherited from Componentable (Component.Flow)

lib/server/model/component.ex, line 119 at r3 (raw file):

  query do

    def by_id(query \\ Component, component_id),

spec

lib/server/model/motherboard.ex, line 198 at r3 (raw file):

      acc -- [component.type]
    end)
    |> case do

Get this case ouf of the pipe

lib/server/model/motherboard.ex, line 268 at r3 (raw file):


    # Now we'll convert the available map into an API-friendly list

extra line

lib/server/model/server.ex, line 41 at r3 (raw file):

  }

  @creation_fields [:type, :motherboard_id]

It's OK to have hostname as a creation field

lib/universe/npc/seed.ex, line 81 at r3 (raw file):


      # Create & attach mobo
      # TODO: Creating NPCs with initial player hardware

Also used here (mention on the issue; also brainstorm on a nice Seed interface in order to declare the expected hardware for an NPC)

priv/repo/server/migrations/20171115093829_hardware_rewrite.exs, line 11 at r3 (raw file):

    create table(:component_specs, primary_key: false) do
      add :spec_id, :string, primary_key: true
      add :data, :jsonb

NOT NULL

priv/repo/server/migrations/20171115093829_hardware_rewrite.exs, line 13 at r3 (raw file):

      add :data, :jsonb
      add :component_type,
        references(:component_types, column: :type, type: :string)

NOT NULL

priv/repo/server/migrations/20171115093829_hardware_rewrite.exs, line 24 at r3 (raw file):

        references(:component_types, column: :type, type: :string),
        null: false
      add :spec_id,

Check whether I'm querying it (there's no index)

priv/repo/server/migrations/20171115093829_hardware_rewrite.exs, line 38 at r3 (raw file):

        ),
        primary_key: true
      add :slot_id, :string, primary_key: true

Pretty sure the slot_id does not need to be a part of the PK

priv/repo/server/migrations/20171115093829_hardware_rewrite.exs, line 57 at r3 (raw file):

    end

    execute """

Mention these changes on the server InitialMigration

test/support/network/helper.ex, line 17 at r3 (raw file):


  @doc """
  Guaranteed to be random.

Beautiful

test/support/server/setup.ex, line 17 at r3 (raw file):

  - entity_id: Specify the entity that owns such server. Defaults to generating
  a random entity.

extra line

test/support/server/component/helper.ex, line 22 at r3 (raw file):


  defp possible_types,
    do: [:cpu, :hdd, :nic, :mobo]

Use Componentable.get_types()

Comments from Reviewable

renatomassaro commented 6 years ago

Reviewed 20 of 159 files at r1, 14 of 65 files at r2, 1 of 7 files at r3, 20 of 22 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable