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: apply fixes to helpers when using set -eE #3285

Merged
merged 1 commit into from Apr 24, 2023

Conversation

georglauterbach
Copy link
Member

Description

For an upcoming PR, these changes are required, because the script that is using the helpers uses set -eE. This leads to situations where errors are not properly handled in our helpers (yet; I plan on changing that in the future).

Fixes issues of an upcoming PR that I do not want to include in the upcoming PR because I am a nice maintainer and I try my best to separate concerns with PRs :D 馃槅

The return 0 statements at the end of some functions seem superflous, but they are not, because the previos command (even when run inside an if-clause) set the return value to non-zero, which is not what we want.

Type of change

  • New feature (non-breaking change which adds functionality)
  • 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

For an upcoming PR, these changes are required, because the script that
is using the helpers uses `set -eE`. This leads to situations where
errors are not properly handled in our helpers (yet; I plan on changing
that in the future).
@georglauterbach georglauterbach added kind/new feature A new feature is requested in this issue or implemeted with this PR area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Apr 24, 2023
@georglauterbach georglauterbach added this to the v12.1.0 milestone Apr 24, 2023
@georglauterbach georglauterbach self-assigned this Apr 24, 2023
@georglauterbach georglauterbach merged commit 7e7497a into master Apr 24, 2023
7 checks passed
@georglauterbach georglauterbach deleted the scripts/fixes branch April 24, 2023 12:35
@casperklein
Copy link
Member

The return 0 statements at the end of some functions seem superflous, but they are not, because the previos command (even when run inside an if-clause) set the return value to non-zero, which is not what we want.

function _adjust_mtime_for_postfix_maincf
{
  if [[ $(( $(date '+%s') - $(stat -c '%Y' '/etc/postfix/main.cf') )) -lt 2 ]]
  then
    touch -d '2 seconds ago' /etc/postfix/main.cf
  fi
  return 0
}

If touch fails for some reason, why would we want to return success (0)?

@georglauterbach
Copy link
Member Author

georglauterbach commented Apr 25, 2023

The return 0 statements at the end of some functions seem superflous, but they are not, because the previos command (even when run inside an if-clause) set the return value to non-zero, which is not what we want.

function _adjust_mtime_for_postfix_maincf
{
  if [[ $(( $(date '+%s') - $(stat -c '%Y' '/etc/postfix/main.cf') )) -lt 2 ]]
  then
    touch -d '2 seconds ago' /etc/postfix/main.cf
  fi
  return 0
}

If touch fails for some reason, why would we want to return success (0)?

It's not the touch - it's the condition in the if-clause that, when the if evaluated to false, would implicitly set the return value to false, i.e. 1, that I was worried about.

Looking at it again, you're right about the if-is-true case though. So a return ${?} after the touch should also be added.

@casperklein
Copy link
Member

when the if evaluated to false, would implicitly set the return value to false, i.e. 1, that I was worried about.

I think you are wrong, that's not the case:

if [ 1 -eq 0 ]; then
        echo "this should never be shown"
fi
echo $? # returns 0

or

if false; then
        echo "this should never be shown"
fi
echo $? # returns 0

@georglauterbach
Copy link
Member Author

You are indeed correct, there is a difference between

set -eE

function x() {
  if false && :; then :; fi
}

x
echo "this will show up"

and

set -eE

function x() {
  false && :
}

x
echo "this wont show up"

I will correct my error and remove unnecessary return 0 statements.

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 kind/new feature A new feature is requested in this issue or implemeted with this PR
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants