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

Fix running script from other directory #11

Merged
merged 4 commits into from
Aug 20, 2024

Conversation

JackPGreen
Copy link
Contributor

@JackPGreen JackPGreen commented Aug 19, 2024

The source assumed that the backport.functions script would always be in the current directory, but as this script's current directory is expected to be a git repo, this is not correct.

Changes:

  • use same dynamic script location resolution as used in test
  • resolve some Shellcheck warnings in that resolution
  • updates exit codes to better follow conventions
  • added unit test

Fixes: #10

@JackPGreen JackPGreen self-assigned this Aug 19, 2024
@JackPGreen JackPGreen marked this pull request as ready for review August 19, 2024 10:06
Copy link
Collaborator

@ldziedziul ldziedziul left a comment

Choose a reason for hiding this comment

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

Backporting is a destructive process (there is no test-repo to work in)

With local mode you can create a local repo git init test-repo and test at least part of the code.

After some thought: with non-local mode we can also test it with a local repo but we need to mock gh command and simply check how it was called. some inspiration (maybe there's something better) https://advancedweb.hu/how-to-mock-in-bash-tests/#mocking-with-function-definitions

As the last resort we can create a hazelcast/backport-tests repo and create there unique branches for tests of each PR but not sure if it's too overengineered.

Whether it fails to run, or runs and prints usage, the exit code is still 1.

Printing usage returns 1 to save us in non-interactive usage from argument mistakes. We also return 1 for cherry-pick problems but maybe we should return 3 instead to distinct it. Where we return 1 in case of success?

@@ -1,7 +1,8 @@
#!/usr/bin/env bash

set -eu
SCRIPT_DIR="$(dirname "$(readlink -f "$0")")"
SCRIPT_DIR=$(readlink -f "$0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why this change?

To fix https://www.shellcheck.net/wiki/SC2312

Consider invoking this command separately to avoid masking its return value (or use '|| true' to ignore).shellcheck(2312)

@JackPGreen
Copy link
Contributor Author

JackPGreen commented Aug 19, 2024

Backporting is a destructive process (there is no test-repo to work in)

With local mode you can create a local repo git init test-repo and test at least part of the code.

After some thought: with non-local mode we can also test it with a local repo but we need to mock gh command and simply check how it was called. some inspiration (maybe there's something better) https://advancedweb.hu/how-to-mock-in-bash-tests/#mocking-with-function-definitions

I think splitting it up and mocking it is sensible, but probably over the top for a simple script.

However, there's still no end-to-end test coverage that the script actually does... anything, which isn't great. But tests didn't exist until last week.

Printing usage returns 1 to save us in non-interactive usage from argument mistakes. We also return 1 for cherry-pick problems but maybe we should return 3 instead to distinct it. Where we return 1 in case of success?

There is some consensus that 2 should be used for these kind of mistakes - grep does that, but tar and find don't.

I've updated it to do that, and then added a test to assert that behaviour which covers this scenario.

1787ae9

test_scripts Outdated Show resolved Hide resolved
Co-authored-by: Łukasz Dziedziul <[email protected]>
@JackPGreen JackPGreen merged commit bb31c8e into hazelcast:master Aug 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script does not run when run from non-working directory
2 participants