-
Notifications
You must be signed in to change notification settings - Fork 27
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
Remove CI image #370
Remove CI image #370
Conversation
993a4c6
to
480b761
Compare
That's a huuuuge improvement! Thanks @jlledom ! I've left some comments. |
function do_license_check() | ||
{ | ||
if [ -n "$CI" ]; then | ||
bundle_exec rake license_finder | ||
bundle exec rake license_finder |
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.
I'm not sure we should run license_finder with bundler. It calls bundler internally for its purposes.
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.
I don't know, it seems correct to me since it's a rake task, and we always run tasks with bundler
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.
weshould run license_finder standalone in fact. This rake thing didn't work well in porta. I don't remember all it was.
Since this is no change, it's fine to merge as is.
P.S. haven't looked at the rake task. If it calls license_finder
as an external command, then it should be fine.
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.
Yes, it does:
Lines 219 to 227 in 2813099
desc 'Check license compliance of dependencies' | |
task :license_finder do | |
STDOUT.puts "Checking license compliance\n" | |
unless system("license_finder --decisions-file=#{File.dirname(__FILE__)}" \ | |
"/.dependency_decisions.yml") | |
STDERR.puts "\n*** License compliance test failed ***\n" | |
exit 1 | |
end | |
end |
@akostadinov @mayorova I think I addressed all your comments. |
To quickly launch Redis services like: single instance, master + replicas and sentinel group
It's causing too much trouble with tests. And I haven't seen any crashes after removing it
:lindex, :lrem and :lset Those are used from Resque but they weren't implemented
We want to have different .env files for development and tests environments. From now on, we should have a `.env.development` and `.env.test` file. They will be loaded in the next order: - Never load any .env file in production - First, try to load `.env.#{ENV['RACK_ENV']}` - If it doesn't exist, try to load .env.test - If doesn't exist, try to load .env
No need for test container anymore
It doesn't require a CI image anymore
end | ||
|
||
begin | ||
return if ENV['RACK_ENV'] == 'production' |
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.
I noticed this thread, but will reply here, because it's relevant to this line.
@akostadinov raised a good point, indeed if we use RACK_ENV=staging
in staging, this part will actually fail, because it will try to require dotenv
, but it's not available in staging.
On the other hand, if we start using RACK_ENV=production
in staging (which I am not opposed to), then we need to add a new environment variable to set the Bugsnag release stage, because currently we are using the value from RACK_ENV
.
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.
Too late!! Now it'll never be fixed!
This closes several issues under #352
.env
files fordevelopment
andtest
(7ec4d82)