search

CLIP-HPC / goslmailer

GoSlurmMailer - drop in replacement for default slurm MailProg. Delivers slurm job messages to various destinations.
MIT License
42 stars 6 forks source link

Hardcoded binary paths fail in certain environments #3

Closed tdido closed 2 years ago

tdido commented 2 years ago

I'm testing goslmailer in my slurm-gcp-based cluster. The system setup differs from the one you've used to develop goslmailer, and therefore some hardcoded paths can't be found causing (somewhat) silent failures.

I've successfully generated an email from a job by changing the following lines to fit my environment:

I'd be happy to work on an PR if you'd let me know what'd be your preferred way to make these paths configurable.

pja237 commented 2 years ago

Hey, thanks for the pointers on "how-not-to-write-code" :smile: and the offer to do PR :+1: Much appreciated.

I guess the quickest way to get rid of this legacy hardcoding might be to add new global vars for these binaries in the global section of the config:

https://github.com/CLIP-HPC/goslmailer/blob/5189f278eae40b4a74117a0afe40ee20fb9520de/internal/config/config.go#L14

Perhaps setting some default values (like the ones hardcoded) if they're not specified.

Then pass ithem on down from the main

https://github.com/CLIP-HPC/goslmailer/blob/5189f278eae40b4a74117a0afe40ee20fb9520de/cmd/goslmailer/goslmailer.go#L65

The lookup one you can also avoid if you don't need it by specifying useLookup: "no" in conf to avoid the getent (quick one) but that should also be propagated from config.

Of course, if you find any better/more elegant way, we're 100% open for suggestions. How does it sound?

tdido commented 2 years ago

Sounds great. I'll try to get a PR going as soon as possible.

pja237 commented 2 years ago

@tdido Thank you for doing the fix for this :+1: I'll add one more PR today and then push a new version out with both.