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

Inventory Enhancements and Reorganisation #105

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

jacderida
Copy link
Contributor

@jacderida jacderida commented Apr 19, 2024

  • 2cbf1de chore: remove terraform check on inventory cmd

    It shouldn't be necessary to have an initialised Terraform workspace to get the inventory for
    deployment, which is the case when using a GHA workflow.

  • 819abb5 refactor: narrow binary option for deployment

    We previously had an SnCodebaseType enum with three variants: Main, Branch and Versioned.
    Now that we can use the release repository to retrieve the latest versions for binaries, I didn't
    see the necessity for a third option; we simply use a version or we build from source. With that in
    mind, I renamed the enum to BinaryOption and provided BuildFromSource and Versioned variants.

    I don't think there should be any functional change here.

  • 08e1d4f refactor: introduce inventory module

    I was finding the main lib module difficult to navigate, so I decided to move inventory-related
    code into its own module, and introduced a new DeploymentInventoryService struct for encapsulating
    the generation of inventory.

    I also made changes and additions in the ansible module, introducing concepts from our own domain
    rather than making it completely generic. We now pass enums for playbooks and inventory values, so
    we don't have to assemble paths all over the place. I think this makes the code more compact,
    readable and maintainable.

  • 7604a20 chore: remove --safe-version arg from deploy

    We don't need to specify the client version when we are using a versioned deployment. It only gets
    used if we are running automated smoke tests, which we don't do at the moment anyway. In any case, I
    updated the smoke-test and upload-test-data commands to supply a --safe-version argument, in
    case we do end up using these again in the future.

    Also changed the use of the Error type in the manage_test_data module in favour of just using
    eyre. It's debatable that this whole crate should be changed to do this and remove the lib
    module.

  • 8720c3f chore: use the node registry to populate inventory

    Now that we have access to the sn_service_management crate, we don't need small structs to
    represent a subset of data from the node registry. We can deserialize the fetched node registries
    directly to a NodeRegistry struct.

    Some other modifications:

    • Fix some Clippy warnings.
    • Rename codebase_type variables to binary_option.
    • Tidy up the sections in the printed report.
    • The BinaryOption is made optional when generating the inventory, because we only have access to
      it on the machine where the deployment initially ran. If it is not supplied, we just use the
      version numbers from the genesis node. We would lose the initial branch information if the testnet
      was deployed using a branch specification, but in most cases that should be for experimental
      testnets.
    • Remove node_count in favour of peers.len().
    • For some reason safenodemand_endpoints was storing a peer ID and socket address pair, but the
      daemon doesn't have a peer ID, so I just removed this.
    • The peer multiaddresses are in the NodeRegistry, so we don't need to fetch these via SSH as an
      additional step.
  • 8f6fc45 chore: review feedback

    • Change inventory_type argument to use a reference to AnsibleInventoryType to avoid cloning in
      loops.
    • Centralise check for the existence of inventory files.
    • Remove irrelevant documentation

It shouldn't be necessary to have an initialised Terraform workspace to get the inventory for
deployment, which is the case when using a GHA workflow.
We previously had an `SnCodebaseType` enum with three variants: `Main`, `Branch` and `Versioned`.
Now that we can use the release repository to retrieve the latest versions for binaries, I didn't
see the necessity for a third option; we simply use a version or we build from source. With that in
mind, I renamed the enum to `BinaryOption` and provided `BuildFromSource` and `Versioned` variants.

I don't think there should be any functional change here.
I was finding the main `lib` module difficult to navigate, so I decided to move inventory-related
code into its own module, and introduced a new `DeploymentInventoryService` struct for encapsulating
the generation of inventory.

I also made changes and additions in the `ansible` module, introducing concepts from our own domain
rather than making it completely generic. We now pass enums for playbooks and inventory values, so
we don't have to assemble paths all over the place. I think this makes the code more compact,
readable and maintainable.
We don't need to specify the client version when we are using a versioned deployment. It only gets
used if we are running automated smoke tests, which we don't do at the moment anyway. In any case, I
updated the `smoke-test` and `upload-test-data` commands to supply a `--safe-version` argument, in
case we do end up using these again in the future.

Also changed the use of the `Error` type in the `manage_test_data` module in favour of just using
`eyre`. It's debatable that this whole crate should be changed to do this and remove the `lib`
module.
@jacderida jacderida force-pushed the remove-terraform-check branch 2 times, most recently from 5ea758f to d5923ba Compare April 19, 2024 22:41
Now that we have access to the `sn_service_management` crate, we don't need small structs to
represent a subset of data from the node registry. We can deserialize the fetched node registries
directly to a `NodeRegistry` struct.

Some other modifications:
* Fix some Clippy warnings.
* Rename `codebase_type` variables to `binary_option`.
* Tidy up the sections in the printed report.
* The `BinaryOption` is made optional when generating the inventory, because we only have access to
  it on the machine where the deployment initially ran. If it is not supplied, we just use the
  version numbers from the genesis node. We would lose the initial branch information if the testnet
  was deployed using a branch specification, but in most cases that should be for experimental
  testnets.
* Remove `node_count` in favour of `peers.len()`.
* For some reason `safenodemand_endpoints` was storing a peer ID and socket address pair, but the
  daemon doesn't have a peer ID, so I just removed this.
* The peer multiaddresses are in the `NodeRegistry`, so we don't need to fetch these via SSH as an
  additional step.
src/ansible.rs Outdated Show resolved Hide resolved
src/ansible.rs Outdated Show resolved Hide resolved
src/logs.rs Outdated Show resolved Hide resolved
* Change `inventory_type` argument to use a reference to `AnsibleInventoryType` to avoid cloning in
  loops.
* Centralise check for the existence of inventory files.
* Remove irrelevant documentation
@jacderida jacderida merged commit 956d9ab into maidsafe:main Apr 23, 2024
5 of 6 checks passed
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.

None yet

2 participants