Use dependabot for linters dependencies #372
Conversation
|
|
||
| [dev-packages] | ||
|
|
||
| [packages] |
There was a problem hiding this comment.
I do not have enough experience with dependabot to confirm it should keep versions in this file up to date, but ideally in any place where dependabot is capable of updating versions for us we should specify exact version numbers. This makes it easier for users in the future to track down the commit associated with a release and see specific versions of dependencies used.
There was a problem hiding this comment.
From some tests, this is fine, as the locked version is on the .lock file.
So here we just say that there is no restriction in the version (like a minimum), and dependabot will correcly update the lock file when new releases are available
There was a problem hiding this comment.
If dependabot cannot update these then leaving * is fine, but if it can then my opinion is we should put the specific versions in here. Another benefit is that new versions for any versioned dependency will have to be approved by a human via a PR like this one https://github.com/github/super-linter/pull/381 which sounds like more human interaction but actually ends up meaning a build is triggered for each new version of the dependency, and so we end up with a lot more testing to keep things running smooth. I think dependabot might even be able to auto merge PRs with passing builds if it becomes a lot to manage, but then the ability to intentionally hold back a specific version is lost. Another reason for doing it in this file and not relying on the lock file is the lock file is a lot less human readable to basic end users. In this case it's pretty small, but in npm for example it is nearly pointless for a human to read the lock file with how complex it is.
There was a problem hiding this comment.
Just to clarify, using the * does not forbid dependabot from working, it works just fine, see my test here
But you still prefer to keep the specific version on the normal file, right? If so, I'll put it with minimum the latest version we are using at the moment, does that seems fine?
There was a problem hiding this comment.
Awesome to see dependabot working in your testing! My only reason for still considering it being in this file is user readability. I don't think it's a sword worth dying on, so if keeping * is easier i'm fine with that.
|
The only thing I think left in this PR to address is https://github.com/github/super-linter/pull/372#discussion_r449727037 Everything else here is looking really good! |
|
https://github.com/github/super-linter/pull/367 merged and seems to have created a conflict. Please rebase. |
|
@GaboFDC this looks good to me! I found a few issues and cleaned them up |
|
Hye @admiralAwkbar thanks for the help. Seems like the rebase was not clean enough, I see duplicate npm install. And the packages install was reverted, I had cleaned it to the minimus as lots of things were not needed anymore. Also, I'll test about https://github.com/github/super-linter/pull/372#discussion_r449727037 to be sure dependabot works as expected. |
|
@GaboFDC yeah that rebase went badly, my bad. Please make any adjustments and push to the branch so we can get it all validated and up to date |
admiralAwkbar
added
automation
GaboFDC
requested review from
admiralAwkbar,
IAmHughes,
jwiebalk and
zkoppert
as code owners
|
GitHub Actions seemed broken earlier today. If you run the following it should re-push your latest commit and trigger new checks: |
|
@GaboFDC Amazing work out there... it needed some cleanup :) |
|
@GaboFDC did you have any more work on this branch or do you feel like you got to where you wanted it to be? |
|
@admiralAwkbar I think this is ready |
This fixes #201
I did a cleanup as good as I can, this is still a draft but only missing part is adding dependabot, that after #367 will only need to add npm and pyenv to the config, so I'll like feedback on the way this was done and any other comments.
About ruby, we need to fix #134 before we can start using the Gemfile properly as I stated in a comment there.
I also used what @nemchik suggested, of copy binaries from maintained docker images, for more clarity, and ease of "install"