-
Notifications
You must be signed in to change notification settings - Fork 1
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
Basic clobberd functionality #8
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sleek, crispy, nice.
Needed to install
|
Quite a lot of unused function warnings on compilation. Guessing that's stubbed/TODO? Mind marking it as such to make the compiler stop whining? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I have some questions and a few nitpicks. Next time I would suggest implementing the base application, and getting that reviewed and merged before adding additional features like pings
. It's unclear what exactly is being added beyond what we've already discussed.
In addition to my comments, I also noticed there's no documentation. Can you amend the README with a user guide, maybe do some comments, and make sure the --help
command has useful info?
The general philosophy I follow while working on house services, especially brand new ones, is generalizing it enough to be useful to anybody, not just a CSH'er.
src/clobberd.rs
Outdated
); | ||
Success | ||
} | ||
#[allow(unreachable_patterns)] //Fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this macro is necessary.
#[allow(unreachable_patterns)] //Fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to prevent a warning, but fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see a specific warning for this when I removed that line and compiled it. Might have to take another look.
} | ||
|
||
pub fn get_config() -> Result<ClobberConfig, String> { | ||
let file = match std::fs::read_to_string("/etc/clobber/config.json") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of hardcoded paths. Make sure you document whatever this file is supposed to be. Better yet, provide an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, me neither, but I don't see how we would have a config file without a hardcoded path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'spose I'm used to container land where everything is in the same directory :P
Still, please do provide some documentation about this.
JobCancelled { .. } => "97f166a0-d9f9-4888-b8ad-b09656f19330", | ||
JobStarted { .. } => "f810410a-0dcd-4271-8b75-b6e6ce5ead7e", | ||
JobFinished { .. } => "aab2a0c6-001c-455a-956f-10c8b685fb87", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm extremely wary of hardcoding magic numbers into applications. What is this, and can it be broken out into a config file in a way that is human readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but this is a temporary measure until I find a not terrible way to put this in a config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another JSON file in the app somewhere, right? Could you use that?
Alternatively: https://crates.io/crates/config
let client = reqwest::blocking::Client::new(); | ||
if let Err(e) = client | ||
.post(format!( | ||
"https://pings.csh.rit.edu/service/route/{}/ping", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should break this URL out into a config file as well.
This code isn't in a fully finished state. At this point, I'm more developing for myself rather than a PR. While this code is (as far as I'm aware) completely functional, it still needs to be tested and it needs to have more CLI operations implemented. |
It'd be easier to review, test, and vet if you split this up into multiple PRs. If you're planning on putting this into production, I'd feel more comfortable if this was done incrementally. If you'd like, I can pull from your branch and take a crack at pairing it down into more review-able PRs. |
The default command, |
Actually, a lot of helps are missing. I don't even know what half of these options do. Please write some docs.
|
@@ -3,5 +3,6 @@ | |||
set -e | |||
|
|||
cargo build --release | |||
sudo install target/release/clobber /usr/local/sbin; echo "Installed clobber." | |||
sudo install target/release/clobber /usr/local/sbin && chmod u+s /usr/local/sbin/clobber && echo "Installed clobber." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clobber should probably go in /usr/local/bin
Notes for later, we need to find an elegant way to read and pass data around info from a config file: https://docs.rs/serde_yaml/latest/serde_yaml/ |
No description provided.