scripts: introduce _log to sedfile

[georglauterbach]

Description

Introduce _log to sedfile, see #2500 (comment)

Type of change

• Improvement (non-breaking change that does improve existing functionality)

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (README.md or the documentation under docs/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[polarathene]

Why is this exactly? It's built in the Dockerfile, are we calling it in a test somewhere outside of the container for some reason?

[georglauterbach]

are we calling it in a test somewhere outside of the container for some reason?

Yes, the sedfile.bats test executes the script directly, not in a DMS container. I think this makes sense though and it's fine - but it also requires this "workaround".

Maybe I should have phrased it differently in the comment: when running the 'sedfile.bats’ test

[polarathene]

I think this makes sense though and it's fine - but it also requires this "workaround".

I don't see anything in the test that requires it to run a copy in the host systems /tmp?

I would think it more appropriate to be testing the sedfile within the image we've built as the test suite already assumes a docker image for pretty much everything else?

---

I'm not going to push for that, it just seems a little out of place is all.

[georglauterbach]

I concur, maybe @casperklein can update this in the future if he agrees as well :)

[casperklein]

We all agree, this solution is ugly. I did not run the sedfile tests in a container, just because at this point there was no reason to do + I am an all-time fan of avoiding unnecessary overhead 😎
I will adjust the bats test in a future PR to run inside a container.

[georglauterbach]

Oops, sorry @polarathene you already had approved, GitHub has let me re-request an already approved review 🤦🏼‍♀️

[casperklein]

I think it does not really matter, if we copy all helpers at this stage. Also, this solution would not introduce another layer to the image.

Suggested change

	COPY ./target/scripts/helpers/log.sh /usr/local/bin/helpers/log.sh
	COPY ./target/scripts/helpers /usr/local/bin/helpers

[polarathene]

I rather we didn't... that was the whole point of making this PR. If you're working on helpers, you have to rebuild the bulk of the image needlessly.

If we get around to a multi-stage build at some point, that'd be less of an issue. Getting all packages installed and ClamAV were both big time sinks to build time I think, plus ClamAV layer requires higher RAM (around 1-2GB IIRC) to build successfully, which usually meant paying twice as much for a VPS or configuring swap on disk. Not that the cost matters much if it's throwaway, and I've just been paying for instances I don't use monthly anyway instead of local VMs which I intend to finally get around to soon 😅

[georglauterbach]

@georglauterbach exactly, you explained why I only added log.sh here :D 👍

[casperklein]

Ack.

If we get around to a multi-stage build at some point,

I proposed that one time (squashing the image), but it was dismissed. IIRC it was because then it's not possible to inspect each layer on the final image anymore. So I do it only for my personal image build.

[georglauterbach]

To be honest, I cannot remember :D Maybe having a multi-stage build with a final, squashed layer at the end is a nice thing to have. If we want to do it in the future, I'll be happy to review it :)

[casperklein]

I think it's somewhere in the maintainers discussions. But without a search function, it's pretty hard to find it 😆

[polarathene]

IIRC it was because then it's not possible to inspect each layer on the final image anymore.

I think that's not true. We can compress directives/layers into groups via multi-stage build, you can copy each one to be a separate layer, not just a single build layer with a 2nd final image copying over the difference to single layer (although that's a common approach).

At least I'm pretty sure that is doable :)

I think there are also more advanced approaches that I haven't investigate yet, beyond default Docker build tooling with BuildKit which is more flexible/featureful, and there are things like Earthly which might be worth a look. I don't mind exploring that once I have tackled my current backlog, but others are welcome to take a stab at it too if they like.

[casperklein]

We all agree, this solution is ugly. I did not run the sedfile tests in a container, just because at this point there was no reason to do + I am an all-time fan of avoiding unnecessary overhead 😎
I will adjust the bats test in a future PR to run inside a container.