Skip to content
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

chore: Add debug group (packages.sh) + more resilient rspamd setup #3578

Merged
merged 10 commits into from
Oct 16, 2023

Conversation

georglauterbach
Copy link
Member

@georglauterbach georglauterbach commented Oct 12, 2023

Description

This PR does 2 things (and I was too lazy to provide two different PRs for these small changes):

  1. Adjust packages.sh to install most and nano and remove ed, adding a dedicated set up packages for debugging (first commit)
  2. Adjust the Rspamd setup script to make it even more robust (commit 2 and 4)

Follow up of #3576

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/)

I despise `nano`, but `(neo)vi(m)` has too many dependencies which would
blow up image size. Moreover, `most` has been added, and I linked it to
`less` because I am not aware of many users knowing `most` so linking it
to less seemed appropriate to me.
As a Rust-programmer, I expect immutability by default. Having it comes
with nice properties. In Bash, you have to explicitly make a variable
immutable. This comment does exactly that for the Rspamd setup.
We now also check whether the variable is unset to provide a more
accurate error message.
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Oct 12, 2023
@georglauterbach georglauterbach added this to the v13.0.0 milestone Oct 12, 2023
@georglauterbach georglauterbach self-assigned this Oct 12, 2023
polarathene
polarathene previously approved these changes Oct 13, 2023
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Mostly opinion below (quoting specific commits), I'm happy with the changes as-is.


add readonly to Rspamd setup script where appropriate
As a Rust-programmer, I expect immutability by default. Having it comes with nice properties.
In Bash, you have to explicitly make a variable immutable.
This comment does exactly that for the Rspamd setup.

You probably meant commit there in the commit message? 😛

While I agree immutability by default is nice, I'm not sure if I'd want to see readonly introduced throughout the codebase. We haven't hit any mutability issues yet right? Granted it's an easier workaround vs migrating from bash scripts to rust (or embeddable / script languages built off rust).

To clarify, readonly after / during variable declaration is just locking it from accidental future modification?


refactor packages.sh and add proper text editor
I despise nano, but (neo)vi(m) has too many dependencies which would blow up image size.
Moreover, most has been added, and I linked it to less because I am not aware of many users knowing most so linking it to less seemed appropriate to me.

Not sure about the symlink idea. It's with good intentions and probably harmless in the common scenario of just calling the binary by its name, but if trying to use some options to launch or navigate (eg: pattern search), this will probably be unexpected and a bit confusing initially to the user. Might result in bug report being raised too.

Personally, unless there's any notable advantage towards the UX for users with the command to troubleshoot within a container, it seems we'd be better off providing the familiar less. Alternatively a docs section that mentions the extra debugging packages we include can mention most there for discoverability and would fit right well on our debugging docs page.

I had a brief look at less vs most and I'm not entirely convinced that it's a better offering (and since the SE discussion was from 2013, not exactly "new"). Not blocking but the suggestion to switch to most gave a different impression 😅

@georglauterbach
Copy link
Member Author

To clarify, readonly after / during variable declaration is just locking it from accidental future modification?

Indeed:

$ readonly --help
readonly: readonly [-aAf] [name[=value] ...] or readonly -p
    Mark shell variables as unchangeable.

    Mark each NAME as read-only; the values of these NAMEs may not be
    changed by subsequent assignment.  If VALUE is supplied, assign VALUE
    before marking as read-only.

I will switch to less now that I read through the websites you linked, because I concur that it is the better alternative as it well-known.

polarathene
polarathene previously approved these changes Oct 13, 2023
casperklein
casperklein previously approved these changes Oct 14, 2023
@polarathene polarathene changed the title misc: adjust packages and Rspamd setup script chore: Add debug group (packages.sh) + more resilient rspamd setup Oct 14, 2023
@georglauterbach georglauterbach merged commit 128e6b4 into master Oct 16, 2023
7 checks passed
@georglauterbach georglauterbach deleted the packages-and-scripts branch October 16, 2023 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scripts kind/improvement Improve an existing feature, configuration file or the documentation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants