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.

Sign up for GitHub

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

Solve bad trap prompt in generate.sh connector-template #5587

Closed
wants to merge 1 commit into from
Closed

Solve bad trap prompt in generate.sh connector-template #5587

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions airbyte-integrations/connector-templates/generator/generate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
error_handler() {
echo "While trying to generate a connector, an error occurred on line $1 of generate.sh and the process aborted early. This is probably a bug."
}
trap 'error_handler $LINENO' ERR
trap 'error_handler $LINENO' EXIT
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do what you'd hope. ERR traps when the prior line was exit code nonzero. EXIT traps any time the script exits, including when it exits successfully with a zero exit code. That means the script can run completely fine and it'll still print the error message after, confusing the user. This has to stay ERR, or be refactored to use a different handling strategy entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jennybrown8 is the suggestion made in this SO post a reasonable approach?

If my understanding of the problem is correct, the issue is that wherever this generate.sh script is getting run the ERR signal does not exist but EXIT does. So trapping exit but only emitting a non zero code upon checking the exit state solves the problem? Maybe there is a better way of doing this?

Very funny that this is a script that runs a docker container. We put the generation code in the docker container to avoid these portability issues... then sure enough the script has a portability issue. (@sherifnada you'll think this is funny)

Copy link
Contributor

@airbyte-jenny airbyte-jenny Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This PR doesn't say what problem it's trying to solve. What was the problem with ERR in the first place?
Oh, found it while digging into the linked issue:
"Try to run connector template got trap: ERR: bad trap. Users are complaining about this."

It's worth trying the stackoverflow suggestion. Do we have an environment where we can exactly reproduce their error? Working on macos, sh is symlinked to bash, so it doesn't behave the same as when run on a system with a real sh shell. This causes difficulty verifying solutions. Can we reproduce their error so we can assure that we've fixed it fully? The state of the code before I added a trap was that it silently fails when a subprocess fails, and the user thinks it succeeded but can't find the generated templates; that was also bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Try to run connector template got trap: ERR: bad trap. Users are complaining about this."

I ran generate.sh on Ubuntu 20.04 and got the same problem as reported by user because of this I tried to suggest this modification. After the change I ran the script again and the ERR messages was gone. The script run correctly both ways, but with trap ERR the message is prompted to user.
Screenshot from 2021-08-24 20-56-54

Copy link
Contributor

Choose a reason for hiding this comment

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

Test it on macos as well. There, if you use trap EXIT, you'll see that after the script runs, and completes successfully, then it prints:

 While trying to generate a connector, an error occurred on line $1 of generate.sh and the process aborted early.  This is probably a bug.

That is, the trap function gets called when the script exits, even when the script succeeds. This is also confusing broken behavior, and not really a solution. We need a better answer, and to confirm that it works under both operating systems, and bash as well as sh.


set -e
_UID=$(id -u)
Expand All @@ -21,7 +21,7 @@ docker build --build-arg UID="$_UID" --build-arg GID="$_GID" . -t airbyte/connec
# Run the container and mount the airbyte folder
if [ $# -eq 2 ]; then
echo "2 arguments supplied: 1=$1 2=$2"
docker run --name airbyte-connector-bootstrap --user $_UID:$_GID -e HOME=/tmp -e package_desc="$1" -e package_name="$2" -v "$(pwd)/../../../.":/airbyte airbyte/connector-bootstrap
docker run --rm --name airbyte-connector-bootstrap --user $_UID:$_GID -e HOME=/tmp -e package_desc="$1" -e package_name="$2" -v "$(pwd)/../../../.":/airbyte airbyte/connector-bootstrap
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine.

else
echo "Running generator..."
docker run --rm -it --name airbyte-connector-bootstrap --user $_UID:$_GID -e HOME=/tmp -v "$(pwd)/../../../.":/airbyte airbyte/connector-bootstrap
Expand Down