-
Notifications
You must be signed in to change notification settings - Fork 5
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: binary installation, checksums, cleanup #181
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
+ Coverage 40.68% 41.26% +0.57%
==========================================
Files 12 12
Lines 757 761 +4
==========================================
+ Hits 308 314 +6
+ Misses 437 434 -3
- Partials 12 13 +1
|
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.
why the PHONY
removal? i understand the chance we have a file named any of the targets is low (in which case i believe it would do nothing), but it's used to indicate non-file targets and is consistent with other repos. it adds minimal noise and is technically correct?
one could open the can-o-worms of should we even use Makefiles to start with?
it was an item on our internal roadmap to consolidate and remove phony usage to bring these all to a consistent standard. dont shoot the messenger! |
i removed the removal of phony. to be honest i dont care if its there, i was just following blindly. |
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.
love the checksum check addition. one suggestion. since it works as-is, i'll approve. happy to re-approve if you decide to take it up.
removes(redacted)PHONY
references in makeinspiration for code changes are from runatlantis/atlantis#4494