dependabot-core icon indicating copy to clipboard operation
dependabot-core copied to clipboard

Add a dependabot implementation for Kubernetes YAML files.

Open brendandburns opened this issue 3 years ago • 3 comments

This addresses #2217

~This is heavily based off of the Dockerfile updater. I basically copied the code + tests and then edited as necessary.~

~There's a lot of duplication, so if there's a better way to factor this (make it part of the docker package?) I'm open to suggestions.~

It is added onto the Docker package. If there is a better way to structure the code, let me know.

Thanks!

brendandburns avatar Jul 06 '22 23:07 brendandburns

We've talked about turning this functionality off with a feature-flag initially, so some pointers on how to do it:

  • Most top-level base classes take an optional options argument, wish accepts a Hash: https://github.com/dependabot/dependabot-core/blob/f614de480d65a4c110815c1c13df8374f6c47f86/common/lib/dependabot/update_checkers/base.rb#L19 https://github.com/dependabot/dependabot-core/blob/f614de480d65a4c110815c1c13df8374f6c47f86/common/lib/dependabot/file_updaters/base.rb#L14

We can pass some sort of kube_manifests_enabled flag to them, and check for it in the code. Here's another piece of functionality (in the npm ecosystem) that's shielded off in that way:

https://github.com/dependabot/dependabot-core/blob/f614de480d65a4c110815c1c13df8374f6c47f86/npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb#L132

PS: I noticed the FileFetcher::Base class currently doesn't take this arg, feel free to add it, as long as it takes a default empty hash def initialize(some:, other:, :args, options: {}) it won't break any code not passing it yet

jurre avatar Aug 03 '22 19:08 jurre

Comments addressed. Please take another look.

brendandburns avatar Aug 09 '22 17:08 brendandburns

@brendandburns thanks! the linting errors should be auto-correctable using bundle exec rubocop -A -c ../.rubocop.yaml from the docker directory, taking a look at the rest now

jurre avatar Aug 10 '22 16:08 jurre

Here's a little change I had to make to get the dry-run script to accept this new arg:

diff --git a/bin/dry-run.rb b/bin/dry-run.rb
index 96c792f06..af1c1e70d 100755
--- a/bin/dry-run.rb
+++ b/bin/dry-run.rb
@@ -489,7 +489,8 @@ $repo_contents_path = File.expand_path(File.join("tmp", $repo_name.split("/")))
 fetcher_args = {
   source: $source,
   credentials: $options[:credentials],
-  repo_contents_path: $repo_contents_path
+  repo_contents_path: $repo_contents_path,
+  options: $options[:updater_options]
 }
 $config_file = begin
   cfg_file = Dependabot::Config::FileFetcher.new(**fetcher_args).config_file

But with that change, it seems to work:

[dependabot-core-dev] ~/dependabot-core $ bin/dry-run.rb docker dsp-testing/dependabot-all-updates-test --dir=/config/kubernetes/default/deployments --updater-options=kubernetes_updates
=> fetching dependency files
=> dumping fetched dependency files: ./dry-run/dsp-testing/dependabot-all-updates-test/config/kubernetes/default/deployments
=> parsing dependency files
=> updating 1 dependencies: nginx

=== nginx (1.14.2)
 => checking for updates 1/1
 => latest available version is 1.23.1
 => latest allowed version is 1.23.1
 => requirements to unlock: own
 => requirements update strategy:
 => updating nginx from 1.14.2 to 1.23.1

    ± nginx.yaml
    ~~~
    19c19
    <         image: nginx:1.14.2
    ---
    >         image: nginx:1.23.1
    ~~~

We have a set of (still) internal integration tests, so I'm setting those up now to make it a bit easier to keep verifying any changes made to this, but overall it looks pretty good

jurre avatar Aug 11 '22 14:08 jurre

@brendandburns I'll extract the changes to pass options to the FileFetcher into a separate PR so I can make some changes to our internal systems that run Dependabot to properly deal with this, do you mind if I push a few small changes to this branch and rebase it on the latest main? I think I should have access to your fork

jurre avatar Aug 12 '22 10:08 jurre

@jurre please go ahead and push your commits to this branch. I will address the style/constant extraction comments after that

brendandburns avatar Aug 12 '22 15:08 brendandburns

I actually went ahead and addressed your comments since they were so small. Please add your commits and then hopefully we can merge this!

brendandburns avatar Aug 12 '22 16:08 brendandburns

Looks like I don't have commit access to the fork actually, all I think we need to do for now is rebase this on main with the changes you've already made, and the bits I pulled out into a separate PR.

After that I'll get those e2e tests set up, test a few more edge cases and I think that's it!

jurre avatar Aug 12 '22 17:08 jurre

@jurre rebased, ptal. Thanks!

brendandburns avatar Aug 15 '22 15:08 brendandburns

Did some extra testing against https://github.com/dsp-testing/dependabot-all-updates-test/blob/main/config/kubernetes/default/multiple-resources.yaml (thanks @bdragon for setting that up), which it seems to handle fine:

 bin/dry-run.rb docker dsp-testing/dependabot-all-updates-test --dir=/config/kubernetes/default --updater-options=kubernetes_updates
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
=> fetching dependency files
=> dumping fetched dependency files: ./dry-run/dsp-testing/dependabot-all-updates-test/config/kubernetes/default
=> parsing dependency files
=> updating 3 dependencies: ruby, nginx, busybox

=== ruby (3.1)
 => checking for updates 1/3
 => latest available version is 3.1.2
 => latest allowed version is 3.1.2
    (no update needed as it's already up-to-date)

=== nginx (1.23)
 => checking for updates 2/3
 => latest available version is 1.23.1
 => latest allowed version is 1.23.1
    (no update needed as it's already up-to-date)

=== busybox (1.35)
 => checking for updates 3/3
 => latest available version is 1.35.0
 => latest allowed version is 1.35.0
    (no update needed as it's already up-to-date)
🌍 Total requests made: '0'

Given that this is behind a feature-gate, I'm inclined to merge it. @bdragon do you see any reason not to?

We'll want to be mindful when writing release notes for dependabot-core, since this is not enabled by default

jurre avatar Aug 16 '22 12:08 jurre

The python spec failure was addressed in https://github.com/dependabot/dependabot-core/pull/5546 and is not related, I'm going to go ahead and merge this change, thanks @brendandburns!

jurre avatar Aug 17 '22 09:08 jurre