Solve bad trap prompt in generate.sh connector-template #5587
Conversation
marcosmarxm
changed the title
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
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.
| @@ -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 | |||
|
|
|
Is there a reason I'm not thinking of for why not fix it like this? #9243 |
What
Closes #5586
How
Handle
trapwith EXIT and not ERR. Also, added--rmflag to handle when running the second time the connector-template.Recommended reading order
x.javay.pythonPre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mddocs/SUMMARY.mddocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described hereUpdating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mddocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing./publishcommand described hereConnector Generator
-scaffoldin their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplatesthen checking in your changes