Basic clobberd functionality #8

ethanf108 · 2023-02-09T20:02:12Z

No description provided.

WillNilges

Sleek, crispy, nice.

WillNilges · 2023-02-21T16:47:56Z

Needed to install openssl-devel to fix this error when I was compiling

  run pkg_config fail: "`\"pkg-config\" \"--libs\" \"--cflags\" \"openssl\"` did not exit successfully: exit status: 1\nerror: could not find system library 'openssl' required by the 'openssl-sys' crate\n\n--- stderr\nPackage openssl was not found in the pkg-config search path.\nPerhaps you should add the directory containing `openssl.pc'\nto the PKG_CONFIG_PATH environment variable\nPackage 'openssl', required by 'virtual:world', not found\n"

  --- stderr
  thread 'main' panicked at '

  Could not find directory of OpenSSL installation, and this `-sys` crate cannot
  proceed without this knowledge. If OpenSSL is installed and this crate had
  trouble finding it,  you can set the `OPENSSL_DIR` environment variable for the
  compilation process.

  Make sure you also have the development packages of openssl installed.
  For example, `libssl-dev` on Ubuntu or `openssl-devel` on Fedora.

  If you're in a situation where you think the directory *should* be found
  automatically, please open a bug at https://github.com/sfackler/rust-openssl
  and include information about your system as well as this message.

  $HOST = x86_64-unknown-linux-gnu
  $TARGET = x86_64-unknown-linux-gnu
  openssl-sys = 0.9.80

  ', /users/u26/wilnil/.cargo/registry/src/github.com-1ecc6299db9ec823/openssl-sys-0.9.80/build/find_normal.rs:191:5
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

WillNilges · 2023-02-21T16:48:48Z

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?

WillNilges

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/clobber.rs

WillNilges · 2023-02-21T16:51:28Z

src/clobberd.rs

+            );
+            Success
+        }
+        #[allow(unreachable_patterns)] //Fallback


I don't think this macro is necessary.

Suggested change

#[allow(unreachable_patterns)] //Fallback

It's to prevent a warning, but fair enough

I didn't see a specific warning for this when I removed that line and compiled it. Might have to take another look.

src/pod.rs

WillNilges · 2023-02-21T16:57:34Z

src/comms.rs

+}
+
+pub fn get_config() -> Result<ClobberConfig, String> {
+    let file = match std::fs::read_to_string("/etc/clobber/config.json") {


Not a huge fan of hardcoded paths. Make sure you document whatever this file is supposed to be. Better yet, provide an example.

Fair, me neither, but I don't see how we would have a config file without a hardcoded path.

'spose I'm used to container land where everything is in the same directory :P

Still, please do provide some documentation about this.

WillNilges · 2023-02-21T17:01:35Z

src/pings.rs

+            JobCancelled { .. } => "97f166a0-d9f9-4888-b8ad-b09656f19330",
+            JobStarted { .. } => "f810410a-0dcd-4271-8b75-b6e6ce5ead7e",
+            JobFinished { .. } => "aab2a0c6-001c-455a-956f-10c8b685fb87",


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?

I agree, but this is a temporary measure until I find a not terrible way to put this in a config file.

There's another JSON file in the app somewhere, right? Could you use that?

Alternatively: https://crates.io/crates/config

WillNilges · 2023-02-21T17:02:34Z

src/pings.rs

+        let client = reqwest::blocking::Client::new();
+        if let Err(e) = client
+            .post(format!(
+                "https://pings.csh.rit.edu/service/route/{}/ping",


You should break this URL out into a config file as well.

src/pings.rs

ethanf108 · 2023-02-21T19:05:54Z

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.

WillNilges · 2023-02-22T03:19:56Z

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.

WillNilges · 2023-03-21T21:55:37Z

The default command, clobber, currently does nothing. It probably ought to print the help, or perhaps a status.

WillNilges · 2023-03-21T21:57:38Z

Actually, a lot of helps are missing. I don't even know what half of these options do. Please write some docs.

jet:~:% clobber kill --help
Usage: clobber kill

Options:
  -h, --help  Print help

jet:~:% clobber watch --help
Usage: clobber watch --device <DEVICE>

Options:
  -d, --device <DEVICE>
  -h, --help             Print help
jet:~:% clobber watch -d
error: a value is required for '--device <DEVICE>' but none was supplied

WillNilges · 2023-03-21T22:07:10Z

res/install.sh

@@ -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."


clobber should probably go in /usr/local/bin

WillNilges · 2023-03-21T22:38:00Z

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/

WillNilges approved these changes Feb 9, 2023

View reviewed changes

ethanf108 added 2 commits February 9, 2023 16:58

Basic clobberd functionality

cd3bb36

Basic socket functionality

155f87a

ethanf108 force-pushed the clobberd branch from 6e35c9a to 155f87a Compare February 9, 2023 22:00

ethanf108 added 4 commits February 9, 2023 17:29

Added some logging

af40591

Added basic socket comm support

94e2259

Basic Systemd service

3abbe43

Added lots of functionality, moved away from threaded approach

c3a6138

ethanf108 force-pushed the clobberd branch from e80dd1a to c3a6138 Compare February 20, 2023 01:34

ethanf108 added 5 commits February 20, 2023 13:28

Fixed bug where containers were immediately marked as finished

b2cc1ef

Added read and write timeout

c68d218

Added full Pings support

0453836

Better error message for read timeout

af24a7c

Added mny commands to clobber

9eac509

WillNilges requested changes Feb 21, 2023

View reviewed changes

ethanf108 added 4 commits February 21, 2023 14:15

Removed dead code, implemented username feature (oops)

2e4e5be

Added Jobs command

a2c5b12

Added My Jobs command

f7c2203

Added setuid (maybe a bad idea)

bf72b3f

Added Status as default command

9c96ddb

WillNilges reviewed Mar 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic clobberd functionality #8

Basic clobberd functionality #8

ethanf108 commented Feb 9, 2023

WillNilges left a comment

WillNilges commented Feb 21, 2023

WillNilges commented Feb 21, 2023

WillNilges left a comment •

edited

Loading

WillNilges Feb 21, 2023

ethanf108 Feb 21, 2023

WillNilges Feb 22, 2023

WillNilges Feb 21, 2023

ethanf108 Feb 21, 2023

WillNilges Feb 22, 2023 •

edited

Loading

WillNilges Feb 21, 2023 •

edited

Loading

ethanf108 Feb 21, 2023

WillNilges Feb 22, 2023

WillNilges Feb 21, 2023

ethanf108 commented Feb 21, 2023

WillNilges commented Feb 22, 2023

WillNilges commented Mar 21, 2023

WillNilges commented Mar 21, 2023

WillNilges Mar 21, 2023

WillNilges commented Mar 21, 2023

Basic clobberd functionality #8

Are you sure you want to change the base?

Basic clobberd functionality #8

Conversation

ethanf108 commented Feb 9, 2023

WillNilges left a comment

Choose a reason for hiding this comment

WillNilges commented Feb 21, 2023

WillNilges commented Feb 21, 2023

WillNilges left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillNilges Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

WillNilges Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanf108 commented Feb 21, 2023

WillNilges commented Feb 22, 2023

WillNilges commented Mar 21, 2023

WillNilges commented Mar 21, 2023

Choose a reason for hiding this comment

WillNilges commented Mar 21, 2023

WillNilges left a comment •

edited

Loading

WillNilges Feb 22, 2023 •

edited

Loading

WillNilges Feb 21, 2023 •

edited

Loading