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

ci: move bazel saucelabs execution to script to be used across all Angular repos #32141

Closed

Conversation

josephperrott
Copy link
Member

No description provided.

@josephperrott josephperrott force-pushed the scriptify-saucelabs branch 3 times, most recently from 4b325bf to b5df73e Compare August 14, 2019 19:34
@josephperrott josephperrott marked this pull request as ready for review August 14, 2019 19:35
@josephperrott josephperrott requested a review from a team as a code owner August 14, 2019 19:35
@josephperrott josephperrott added the target: patch This PR is targeted for the next patch release label Aug 14, 2019
@josephperrott josephperrott changed the title ci: move bazel stackblitz execution to script to be used across all Angular repos ci: move bazel saucelabs execution to script to be used across all Angular repos Aug 14, 2019
@@ -0,0 +1,189 @@
#!/usr/bin/env bash

set -u -o pipefail
Copy link
Member

Choose a reason for hiding this comment

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

No -e? 😱

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have -e on this script because we explicitly want to make sure we continue on errors so that we make sure we close the tunnel everytime.

For instance, if the bazel test fails, we don't want its erring exit code to cause the entire script to exit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems dangerous. If you're wanting to close the tunnel if the bazel test fails you can set +e set -e around the bazel eval and check its exit code manually so that the script continues and closes the tunnel. You still want the script to exit with the exit code of the bazel test in that case so CI reports the test failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

That works too, updated.

function touch-safe {
for f in "$@"; do
[ -d $f:h ] || mkdir -p $f:h && command touch $f
done
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent indentation 😱

# acquire CircleCI instances for too long if sauceconnect failed, we need a connect timeout.
readonly SAUCE_READY_FILE_TIMEOUT=120

# Create saucelabs log file it doesn't already exist.
Copy link
Member

Choose a reason for hiding this comment

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

Missing "if".

badCommandSyntax=1;
elif [[ ! $USER_COMMAND =~ ^(yarn bazel) ]]; then
echo "The command provided must be a bazel command run via yarn, beginning with \"yarn bazel\"";
badCommandSyntax=1
Copy link
Member

Choose a reason for hiding this comment

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

No semicolon? 🤔

(
cd $GIT_ROOT_DIR && \
# Run bazel command with saucelabs specific environment variables passed to the action
eval "$USER_COMMAND --define=KARMA_WEB_TEST_MODE=SL_REQUIRED \
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're looking to drop this script into other repos, note that the --define=KARMA_WEB_TEST_MODE=SL_REQUIRED is specific to the angular repo and it corresponds to the browsers defined by /browser-providers.conf.js and consumed by /karma-js.conf.js.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commented, noting this fact

# The root directory of the git project the script is running in.
readonly GIT_ROOT_DIR=$(git rev-parse --show-toplevel 2> /dev/null)
# Location for the saucelabs log file.
readonly SAUCE_LOG_FILE=/tmp/angular/sauce-connect.log
Copy link
Contributor

Choose a reason for hiding this comment

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

could use mktemp (https://www.gnu.org/software/autogen/mktemp.html) instead to make this generic and not angular specific

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should be fine to leave it as angular for now since our main use case is on CI and its only being used for angular repos as of now. If we were to set this up as fully available to anyone we would need to address it though.

@josephperrott josephperrott force-pushed the scriptify-saucelabs branch 2 times, most recently from 424eb3c to 46842ed Compare August 15, 2019 20:32
@josephperrott josephperrott added the action: merge The PR is ready for merge by the caretaker label Aug 16, 2019
AndrewKushnir pushed a commit that referenced this pull request Aug 16, 2019
@josephperrott josephperrott deleted the scriptify-saucelabs branch August 19, 2019 19:51
ngdevelop-tech pushed a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
sabeersulaiman pushed a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants