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

audit: Port audit_checksum method to rubocop and add tests #2755

Merged
merged 2 commits into from Jun 9, 2017

Conversation

Projects
None yet
3 participants
@GauthamGoli
Copy link
Member

GauthamGoli commented Jun 8, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

#569

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Great work! A couple of nits here.

end
end

def audit_checksums(node, spec, resource = nil)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2017

Member

Maybe resource_name?

problem "#{msg_prefix}sha256 contains invalid characters"
end

problem "#{msg_prefix}sha256 should be lowercase" unless regex_match_group(checksum, /^[a-f0-9]+$/)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2017

Member

If this line is >80 characters: use unless ... do

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 8, 2017

Member

Can I make it return unless and followed by problem?

This comment has been minimized.

@MikeMcQuaid
problem "#{msg_prefix}sha256 should be 64 characters"
end

unless regex_match_group(checksum, /^[a-fA-F0-9]+$/)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2017

Member

Maybe make it a case-sensitive regex (/i) and skipA-F?

This comment has been minimized.

@JCount

JCount Jun 8, 2017

Contributor

Just to clarify for posterity, you do mean case-insensitive regex using /i, correct?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2017

Member

Sorry: yes, thanks @JCount

@@ -250,7 +256,7 @@ def caveats_strings

# Returns the array of arguments of the method_node
def parameters(method_node)
return unless method_node.send_type?
return unless method_node.send_type? || method_node.block_type?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 8, 2017

Member

Turn this into an if (or two); unless .. || is hard to parse.

@GauthamGoli GauthamGoli force-pushed the GauthamGoli:audit_checksum_rubocop branch from 3b96b6c to d09d5ec Jun 8, 2017

@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Jun 9, 2017

@MikeMcQuaid a side point: if sha256 gets deprecated / new hashes in future, then this would require refactoring. Should I refactor line 34 - 51 to not hardcode sha256?
Edit: May not be required as new hash would mean, just adding new conditions below line 51.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 9, 2017

@MikeMcQuaid a side point: if sha256 gets deprecated / new hashes in future, then this would require refactoring. Should I refactor line 34 - 51 to not hardcode sha256?
Edit: May not be required as new hash would mean, just adding new conditions below line 51.

Nah, I think it's fine as-is; we'll be on sha256 for quite a while longer.

@MikeMcQuaid MikeMcQuaid merged commit e83e394 into Homebrew:master Jun 9, 2017

3 checks passed

codecov/patch Coverage not affected when comparing b6e1dde...d09d5ec
Details
codecov/project 65.35% (+0.16%) compared to b6e1dde
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 9, 2017

@GauthamGoli Great work again, thanks!

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Have left a few comments that led to the revert. Test this on the wget and angular-cli formulae. Thanks!

module FormulaAudit
class Checksum < FormulaCop
def audit_formula(_node, _class_node, _parent_class_node, body_node)
%w[Stable Devel HEAD].each do |name|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

Let's just make these lowercase, I don't think Devel looks very nice.

next unless spec_node = find_block(body_node, name.downcase.to_sym)
_, _, spec_body = *spec_node
audit_checksums(spec_body, name)
resource_blocks = find_all_blocks(spec_body, :resource)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

We also need to get all resource blocks that aren't inside a stable/devel/head.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 9, 2017

Member

This is not currently being done in audit_specs, right?

This comment has been minimized.

@JCount

JCount Jun 9, 2017

Contributor

I believe that the resource blocks are currently being audited as part of running audit_specs, but the code structure relies on the ResourceAuditor class, and the audit_checksum it contains. I think that gap might be part of the cause of this issue.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 10, 2017

Member

I couldn't find any existing method/code in audit.rb which audits resource blocks that aren't inside stable/devel/head blocks, is this something we want to add?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 10, 2017

Member

Auditing the stable spec audits everything which is outside those blocks (as they are implicitly stable). The existing code is therefore fine, it's just this code that needs adjusted for that implicit stable resource handling. Make sense?

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 12, 2017

Member

Yeah. Got it. Thanks.

problem "#{msg_prefix}SHA1 checksums are deprecated, please use SHA256"
end

checksum_node = find_node_method_by_name(node, :sha256)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

Need to handle if this is nil; I hit this locally. Also, this doesn't seem to be constrained to the current node as far as I can tell.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 9, 2017

Member

Should an error be raised, if there is no sha256 specified?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

A RuboCop error: yes, I think so. I was seeing this on formulae that had them though so it may be a bug, too.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 10, 2017

Member

Do you mean, find_node_method_by_name returns nil even when there is sha256 in node?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 10, 2017

Member

I can't remember now helpfully but I think it's just that again we can have a stable/devel/head block that doesn't have a URL therefore doesn't need a checksum (although head urls don't need checksums either).

problem "#{msg_prefix}sha256 contains invalid characters"
end

return unless regex_match_group(checksum, /^[a-f0-9]+$/)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

return unless regex_match_group(checksum, /[A-F]/) seems correct for here.

This comment has been minimized.

@GauthamGoli

GauthamGoli Jun 9, 2017

Member

Ah. This is embarrassing :/

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 9, 2017

Member

We all make mistakes; it's embarrassing that I didn't spot it in review!

@GauthamGoli GauthamGoli deleted the GauthamGoli:audit_checksum_rubocop branch Jun 12, 2017

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.