feat(agents): address killswitch usability issues #246
Conversation
lattejed
force-pushed
the
lattejed/killswitch_usability
branch
from
6d1734f
to
68ed945
Compare
| @@ -111,7 +111,7 @@ impl ChainConf { | |||
| }) | |||
| .unwrap_or_else(|_| { | |||
| tracing::debug!("falling back to ethereum"); | |||
| "etherum".to_owned() | |||
| "ethereum".to_owned() | |||
There was a problem hiding this comment.
lmao i have made this typo so many times 😬
|
|
||
| static CLIENT: OnceCell<Client> = OnceCell::new(); | ||
| static KMS_CLIENT: OnceCell<KmsClient> = OnceCell::new(); | ||
| static CLIENT: OnceCell<Client> = OnceCell::const_new(); |
There was a problem hiding this comment.
can you comment on this change? I'm assuming that it allows async oncecells?
There was a problem hiding this comment.
Correct, loading {KMS_}CLIENT from more than one thread would cause all threads to try to build a client and all but the first to complete to panic.
|
|
||
| /// `Error` for KillSwitch | ||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum Error { | ||
| /// Cannot find local AWS credentials | ||
| #[error("BadCredentials: Cannot find AWS credentials: {0}")] |
There was a problem hiding this comment.
do these messages add any error info that is not already contained in the inner error's message?
There was a problem hiding this comment.
I wanted these to have more context and err on the side of being too verbose
tools/killswitch/src/killswitch.rs
Outdated
| let channels = if args.all { | ||
| Self::make_channels(&settings) | ||
| } else { | ||
| let destination_network = args | ||
| .all_inbound | ||
| .clone() |
There was a problem hiding this comment.
looks like it could be as_ref instead of clone?
| const AWS_CREDENTIALS_PROFILE_PRODUCTION: &str = "nomad-xyz-prod"; | ||
| const CONFIG_S3_BUCKET_DEVELOPMENT: &str = "nomad-killswitch-config-dev"; | ||
| const CONFIG_S3_BUCKET_PRODUCTION: &str = "nomad-killswitch-config-prod"; | ||
| const CONFIG_S3_KEY: &str = "config.yaml"; |
There was a problem hiding this comment.
naming seems off. is this a key or a filepath?
There was a problem hiding this comment.
That's how S3 defines them. Agreed though
|
I've already changed that unnecessary |
* Add rusoto * Add credentials error * Add validation for AWS creds * Add serde_yaml and reqwest * Add remote secrets helper * Add deserialize test for secrets * Add test helper * Combine cred fetch and validate * Combine secrets and aws. Add mocked S3 test for secrets * Remove aws * Fix typo in network name * Add handler for setting env vars * Fix config path * Add new errors * Remove output * Make shared KMS client thread-safe * Make Settings Clone * Make killswitch run in parallel and in a single pass by channel * Make output human-readable and more desriptive, update cli args * Fix tests * Add env var test * Improve docs * Fix warnings * Bump clap version * Fix test * Change how local secrets path is picked up for testing * Update environment options * Output should print full txid * Run CI on ubuntu 22.04 * Add S3 info * Update CHANGELOG * Update README * Change S3 bucket name * Differentiate between S3 environments, update names * Remove uncessary clone
Motivation
Killswitch is functional but takes too long to configure. Configuration should be handled automatically and require only local AWS credentials to run.
Solution
--environmentflag to select deployment to target (e.g., production)--forceflag (the opposite of a--dry-runflag)PR Checklist