-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
remove-terraform-check
branch
2 times, most recently
from
April 19, 2024 22:41
5ea758f
to
d5923ba
Compare
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.
jacderida
force-pushed
the
remove-terraform-check
branch
from
April 19, 2024 22:43
d5923ba
to
8720c3f
Compare
jacderida
force-pushed
the
remove-terraform-check
branch
from
April 23, 2024 20:12
66a76a5
to
74eb0cc
Compare
RolandSherwin
approved these changes
Apr 23, 2024
* 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
force-pushed
the
remove-terraform-check
branch
from
April 23, 2024 20:32
74eb0cc
to
8f6fc45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2cbf1de chore: remove terraform check on
inventory
cmdIt 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
andVersioned
.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 providedBuildFromSource
andVersioned
variants.I don't think there should be any functional change here.
08e1d4f refactor: introduce
inventory
moduleI was finding the main
lib
module difficult to navigate, so I decided to move inventory-relatedcode into its own module, and introduced a new
DeploymentInventoryService
struct for encapsulatingthe generation of inventory.
I also made changes and additions in the
ansible
module, introducing concepts from our own domainrather 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 fromdeploy
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
andupload-test-data
commands to supply a--safe-version
argument, incase we do end up using these again in the future.
Also changed the use of the
Error
type in themanage_test_data
module in favour of just usingeyre
. It's debatable that this whole crate should be changed to do this and remove thelib
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 torepresent a subset of data from the node registry. We can deserialize the fetched node registries
directly to a
NodeRegistry
struct.Some other modifications:
codebase_type
variables tobinary_option
.BinaryOption
is made optional when generating the inventory, because we only have access toit 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.
node_count
in favour ofpeers.len()
.safenodemand_endpoints
was storing a peer ID and socket address pair, but thedaemon doesn't have a peer ID, so I just removed this.
NodeRegistry
, so we don't need to fetch these via SSH as anadditional step.
8f6fc45 chore: review feedback
inventory_type
argument to use a reference toAnsibleInventoryType
to avoid cloning inloops.