-
Notifications
You must be signed in to change notification settings - Fork 47
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 s3 specs - part 2 #1539
Fix s3 specs - part 2 #1539
Conversation
I'll check master again once this is merged to make sure the AWS specs are running there again. |
config.filter_run_excluding(aws: true) unless ENV['CI'] && ENV['TRAVIS_PULL_REQUEST_SLUG'].match('samvera-labs/hyku') | ||
# is where the valid aws keys live. TRAVIS_PULL_REQUEST_SLUG is "" when the job is a push job | ||
unless ENV['CI'] && | ||
(ENV['TRAVIS_PULL_REQUEST_SLUG'].match('samvera-labs/hyku') || ENV['TRAVIS_PULL_REQUEST_SLUG'].blank?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, could you just remove the TRAVIS_PULL_REQUEST_SLUG
entirely? I'm not understanding what situation this is trying to prevent. Maybe you could modify the commit message to explain a bit more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcoyne here is the description from part 1. The issue is that only the travis ci from this repo is set to run the S3 specs successfully. So all forked pull request spec sets fail on the S3 connection tests. BUT we want those test to run after a branch is merged to make sure there is no regression. This commit prevents the tests from running locally (as it was before) and from running on forks, but makes sure they run on pull requests from within this repo and on merged branches. This is part 2, simply because I can't test that the specs actually run like they are supposed to until it is merged it is a little tricky to know if we got the variables right for all the cases w/o getting a merge =-)
Fixes #1524
No longer run S3 related specs on non-samvera-labs repos. This will allow specs to pass on forks.
This is a first swing at getting Travis to not run the specs unless the code is in samvera-labs/hyku. Specs on forks will thus pass and the 4 failing S3 specs from forks will get run properly once code is merged.
Note - need to check that the specs in question are run after this is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. 👍
Do you want to squash these commits together? |
0acbfb0
to
1642ec6
Compare
1642ec6
to
ff47cc0
Compare
@jcoyne finally got back to squashing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me.
As hypothesized, the specs are not running in master any more. That appears to be because TRAVIS_PULL_REQUEST_SLUG if you are not talking about a pull request. This should work in both cases now.