search

ashfurrow / danger-ruby-swiftlint

A Danger plugin for SwiftLint.
https://rubygems.org/gems/danger-swiftlint
MIT License
203 stars 80 forks source link

Add integrity validation when downloading Swiftlint (#162) #170

Closed NicolasCombe5555 closed 3 years ago

NicolasCombe5555 commented 3 years ago

Hello 👋🏻,

I added integrity verification concerning #162 👍🏻. However I am not sure if it is okay for me to move the commands that were inside sh [ ] before, I did that because I was having trouble with the if else multiline command. Let me know if I need to change that. Also if the names of the environment variables are okay or if I should not use them env variables for this at all.

I tested this locally and it works 🚀, also I added the -o flag to unzip to avoid the prompt to override the previous file. I saw that the issue requested a respective unit test as well but I do not have much ruby experience and I could not find where or how to make that unit test. My first thought was 2 tests: one that downloads and succeeds and another one that fails. I was hoping someone could give me a hint on how to start the tests.

Thanks 👍🏻

NicolasCombe5555 commented 3 years ago

@ashfurrow I deleted the frozen~ line and I also went for the arguments instead of env variables, I believe arguments are safer and cleaner, thanks for the suggestion. Let me know if this looks good. Also thanks @dirtyhabits97 for the observation.

I tested this for success:

and for failure:

👍🏻🚀

ashfurrow-peril[bot] commented 3 years ago

Thanks a lot for contributing @NicolasCombe5555! You've been invited to be a collaborator on this repo – no pressure to accept! If you'd like more information on what this means, check out the Moya contributor guidelines and feel free to reach out with any questions.

Generated by :no_entry_sign: dangerJS

ashfurrow commented 3 years ago

Okay! Thanks again – this has been released in version 0.29.0. Let me know when you have a chance to test it out 👍

NicolasCombe5555 commented 3 years ago

@ashfurrow So @dirtyhabits97 reached out, letting me know the swiftlint:install rake task step was failing. I gave it a look and it was failing because of a -> Command failed with status (127) error, I did a bit of research and I got to the conclusion that the script did not have execution capabilities or was not being found. I also found this as reference similar-issue. The fix is one line in line 18 inside ext/swiftlint/Rakefile.

sh "./downloadSwiftlint.sh -u #{URL} -d #{DESTINATION} -a #{ASSET} -dh #{SWIFTLINT_MD5_HASH}" <- old line sh "sh downloadSwiftlint.sh -u #{URL} -d #{DESTINATION} -a #{ASSET} -dh #{SWIFTLINT_MD5_HASH}" <- new line

Should I create a new pull request for this? Worth to mention that this problem does not exist locally.

ashfurrow commented 3 years ago

Gotcha, okay. Let's open up a new PR for this 👍