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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

scripts: touchups for v12.0.0 #3144

Merged
merged 6 commits into from
Mar 4, 2023
Merged

scripts: touchups for v12.0.0 #3144

merged 6 commits into from
Mar 4, 2023

Conversation

georglauterbach
Copy link
Member

Description

Touchups for the upcoming v12.0.0 release. Each commit resolved a small nitpick. Feel free to add yours as well! Should be easily reviewable commit-by-commit. Only small changes allowed 馃槈

Fixes #

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

@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Mar 3, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Mar 3, 2023
@georglauterbach georglauterbach self-assigned this Mar 3, 2023
@georglauterbach georglauterbach added the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Mar 3, 2023
@georglauterbach
Copy link
Member Author

georglauterbach commented Mar 3, 2023

Test failure is expected; waiting for #3134 to get merged.

@georglauterbach georglauterbach removed the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Mar 3, 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 馃憤

Small suggestion that might make sense with ONE_DIR=1

target/scripts/startup/setup.d/mail_state.sh Show resolved Hide resolved
Comment on lines +84 to +88
elif [[ ${ONE_DIR} -eq 1 ]]
then
_log 'warn' "'ONE_DIR=1' but no volume was mounted to '${STATEDIR}'"
else
_log 'debug' 'Not consolidating state (because it has been disabled)'
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked into it, but since ONE_DIR=1 is default, if the only usage is here now, perhaps variables.sh should default to ONE_DIR=0 if ENV is unset, and only change it to ONE_DIR=1 if /var/mail-state exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did I miss something here? ONE_DIR is used above in conjunction with && [[ -d ${STATE_DIR} ]] - this is an elif case, not an if case 馃檪

Copy link
Member

Choose a reason for hiding this comment

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

Did I miss something here?

The warning would always be output with variables.sh setting ONE_DIR=1 as default? (unless there is an explicit opt-out with ONE_DIR=0).

EDIT: For UX you can ignore the below content. Your decision here is the right way to go 馃憤 (opt-out of warning via explicit ONE_DIR=0)


I think I had another comment in this thread that didn't seem to get added during review 馃槵

I was proposing variables.sh to set ONE_DIR=1 if /var/mail-state existed, so that the warning here would not show up by default.

  • The test suite doesn't run with a /var/mail-state volume in any tests, so we technically use ONE_DIR=0.
  • It used to be opt-in ENV until the default was changed. Since we expect it at /var/mail-state anyway, we could just derive it that way.
  • ONE_DIR is not used anywhere else in our scripts, just special handling in mail_state.sh.
  • The warning then becomes redundant, opt-in via existence of /var/mail-state.

Then again, if there is no explicit opt-out with ONE_DIR=0, for new users especially it probably does make sense to display a warning 馃憤

Copy link
Member

Choose a reason for hiding this comment

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

The error message is not triggerd, when ENABLE_SRS=1 and ONE_DIR=1 is set and no volume is mounted, because the directory is created in the setup process here:

install -d -m 0775 "${POSTSRSD_STATE_DIR}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Any idea how to solve that?

Copy link
Member

Choose a reason for hiding this comment

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

SRS seems pretty broken to me. For example, SRS stores stuff in ${POSTSRSD_STATE_DIR}, however there is no symlink pointing to it..
I will take a closer look later.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #3158

@georglauterbach georglauterbach merged commit 8ec5dbe into master Mar 4, 2023
@georglauterbach georglauterbach deleted the touchups/v12 branch March 4, 2023 09:57
@casperklein casperklein mentioned this pull request Mar 5, 2023
11 tasks
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants