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

Use bundle-audit rake task from the gem #831

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Use bundle-audit rake task from the gem #831

merged 1 commit into from
Mar 14, 2017

Conversation

odlp
Copy link

@odlp odlp commented Mar 10, 2017

Instead of defining the audit task from scratch, we can now import the task from the gem itself.

One caveat: The task name changes from bundler:audit to bundle:audit as defined by the gem. We could make an alias between the old & new command but I suspect most use is just via the default Rake task.

Instead of defining the audit task from scratch, we can import
the task from the gem itself.
end
end
end
require "bundler/audit/task"

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -29,6 +29,14 @@
end
end

it "includes the bundle:audit task" do

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -401,7 +401,7 @@ def setup_segment

def setup_bundler_audit
copy_file "bundler_audit.rake", "lib/tasks/bundler_audit.rake"
append_file "Rakefile", %{\ntask default: "bundler:audit"\n}
append_file "Rakefile", %{\ntask default: "bundle:audit"\n}

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.
%-literals should be delimited by ( and ).

Copy link
Contributor

@croaky croaky Sep 8, 2017

Choose a reason for hiding this comment

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

Should this be task default: ["spec", "bundle:audit"]?

Copy link
Author

@odlp odlp Sep 11, 2017

Choose a reason for hiding this comment

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

Although it may be clearer what's going on defined as one line, is it lumping together responsibilities? I think we should augment the default task with bundle:audit independently of expecting the specs to run.

Having said that, I did find Rake's behaviour of find the append (rather-than-redefine) mechanism confusing at first... it's not clear.

# Rakefile

task :foo do
  puts "FOO"
end

task :bar do
  puts "BAR"
end

task(:default).clear
task default: :foo
task default: :bar
$ bundle exec rake
FOO
BAR

One way we could make this clearer is writing it as:

Rake::Task[:default].enhance ["bundle:audit"]

Then there's less confusion whether the task is being overwritten or modified.

Copy link
Author

Choose a reason for hiding this comment

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

@croaky also this comment on the original PR for context: #831 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Although it may be clearer what's going on defined as one line, is it lumping together responsibilities? I think we should augment the default task with bundle:audit independently of expecting the specs to run.

I think I'd expect rake in a Suspender-ized app to run spec then bundle:audit at this point. If that's what is happening via multiple task default: invocations, that's great.

Related: I learned about https://github.com/presidentbeef/brakeman last week. Seems like it covers security cases additional to Bundle Audit. Might be worth experimenting with on some apps.

Copy link
Contributor

Choose a reason for hiding this comment

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

@croaky we are using brakeman in our suspenders fork. I can cherry-pick and create a PR if you are interested!

end
end
end
require "bundler/audit/task"

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -29,6 +29,14 @@
end
end

it "includes the bundle:audit task" do

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@@ -401,7 +401,7 @@ def setup_segment

def setup_bundler_audit
copy_file "bundler_audit.rake", "lib/tasks/bundler_audit.rake"
append_file "Rakefile", %{\ntask default: "bundler:audit"\n}
append_file "Rakefile", %{\ntask default: "bundle:audit"\n}

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.
%-literals should be delimited by ( and ).

@odlp
Copy link
Author

odlp commented Mar 10, 2017

Disregarding the Hound single-quoted strings warnings because the changes match the current formatting of the files changed, and the thoughtbot Ruby styleguide states:

Prefer double quotes for strings.

We could disable this check by adding a .rubocop.yml entry for this rule like this.

Copy link
Contributor

@croaky croaky left a comment

Choose a reason for hiding this comment

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

Nice! Do we need to update any generated RSpec task to reference bundle:audit now instead of bundler:audit? I think we configure it to run after the suite passes.

@croaky
Copy link
Contributor

croaky commented Mar 10, 2017

I like the idea of switching the Hound config to prefer double quotes strings and moving toward that over time as files are touched.

@odlp
Copy link
Author

odlp commented Mar 10, 2017

@croaky yes, the default Rake task (which has :spec defined a few lines above), gets the "bundle:audit" task appended to it. I've updated that as part of the PR on this line.

So ultimately the Rakefile in a new Suspenders project looks like this:

# Add your own tasks in files placed in lib/tasks ending in .rake,
# for example lib/tasks/capistrano.rake, and they will automatically be available to Rake.

require_relative 'config/application'

Rails.application.load_tasks
task(:default).clear
task default: [:spec]

if defined? RSpec
  task(:spec).clear
  RSpec::Core::RakeTask.new(:spec) do |t|
    t.verbose = false
  end
end

task default: "bundle:audit"

So task default ends up being [:spec, "bundle:audit"] because Rake appends to a pre-declared task.

@odlp odlp merged commit ae2281e into master Mar 14, 2017
@tysongach tysongach deleted the op-bundler-audit branch May 8, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants