Closed dunn closed 7 years ago
Ah, forgot all the new stuff needs to be in a if [ "$TRAVIS_PULL_REQUEST" = "false" ]
block: https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
Instead of doing big conditional blocks in Travis, I might pull the new steps into a shell script.
Not as pretty, but it works.
Thanks for writing this! I think TRAVIS_PULL_REQUEST
isn't the right thing to check. For instance, I often use pull requests for my own code. Among other things, it's a handy way to make sure tests pass. Instead, I would check whether the credentials are available in env.
Also, I think we should run all the steps unconditionally, except the ones that will fail without credentials. For instance, npm install
is fine, but npm test
is expected to fail.
Thanks for writing this! I think TRAVIS_PULL_REQUEST isn't the right thing to check. For instance, I often use pull requests for my own code. Among other things, it's a handy way to make sure tests pass. Instead, I would check whether the credentials are available in env.
For PRs from branches that are part of your own repository, Travis will build them twice as long as "Build pushes" is turned on; once as a PR and once as a push. But I also thought encrypted variables weren't available for any PRs, when they are only hidden from PRs coming from forks. So I'll update to check for the variable(s).
Also, I think we should run all the steps unconditionally, except the ones that will fail without credentials. For instance, npm install is fine, but npm test is expected to fail.
What would be the benefit of running npm install
?
What would be the benefit of running npm install?
It makes the .travis.yml simpler, so there are fewer places where a reader has to say "Why is there a conditional here?"
OK, fair enough.
Sweet, we're passing again: https://travis-ci.org/dunn/blocktogether/builds/157387268
Looks like you forgot to git add ./bin/travis/install.sh
🙀
Yeah, this is the error I saw before: https://travis-ci.org/dunn/blocktogether/jobs/157389915#L277
I think this is the cause:
https://stackoverflow.com/questions/26299552/travis-sudo-is-disabled
I'm guessing that when you put sudo
in the .travis.yml, Travis figures it out and automatically disables the Docker-based infrastructure, but when it happens only in the shell script, Travis doesn't know, so the job runs on the Docker-based infrastructure where sudo is not enabled. Instead of including sudo in the Travis file, let's add:
sudo: true
to the config. I think we can do some refactoring to make things work without sudo, but that's work for another change.
Looks like ${CONSUMER_KEY}
is returning true: https://travis-ci.org/jsha/blocktogether/jobs/157390801
🌲 "[ ${NONEXISTENT} ] && echo hello"
-bash: [ ] && echo hello: command not found
Oh, it's the quotes messing it up.
Yep - there are two layer of interpolation here, I think. The string gets evaluated once, interpolating ${NONEXISTENT}
to be the empty string. Then that becomes what gets run, which doesn't have a token in that position.
Did you conclude that Travis' YAML parser really does need those quotes?
Did you conclude that Travis' YAML parser really does need those quotes?
Yeah, I tried without and it died with the same error message: https://travis-ci.org/dunn/blocktogether/builds/157388926
I got 1/2 passing this time: https://travis-ci.org/dunn/blocktogether/builds/157391226. I went back to having them run simultaneously, but I'm not sure why that would matter.
Oh right, I forgot! Ok, I'm convinced that the if;then
approach works better here.
Re-ran the failing job and it passed.
Great. LGTM, will merge later tonight when I have a chance to set up the env vars correctly. Thanks so much!
On Sat, Sep 3, 2016 at 10:28 PM, Alex Dunn notifications@github.com wrote:
Re-ran the failing job and it passed.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/jsha/blocktogether/pull/229#issuecomment-244580622, or mute the thread https://github.com/notifications/unsubscribe-auth/AANcLYcJaXQVOGoV7Ttx1SG1-5E2E8R3ks5qmizIgaJpZM4JyPYd .
Per https://docs.travis-ci.com/user/environment-variables/:
if it does not contain sensitive information, might be different for different branches and should be available to forks – add it to your .travis.yml if it does contain sensitive information, and might be different for different branches – encrypt it and add it to your .travis.yml if it does contain sensitive information, but is the same for all branches – add it to your Repository Settings
The third case is true, so I'm going to set the vars in Repository Settings, then merge this, then remove the env vars lines from .travis.yml.
Great!
The secure environment variables need to be refreshed:
BT_TEST_MAIN_USER
BT_TEST_MAIN_PASS
CONSUMER_KEY
CONSUMER_SECRET
I used the
travis
gem:and so on.