-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for bumping docker container versions referenced from within Helm files #5738
Conversation
@brendandburns : The docker |
@honeyankit thanks I'll take a look. (is there a way that I can run these locally?) |
@honeyankit style issues addressed (I hope) ptal |
https://github.com/dependabot/dependabot-core#running-the-tests describes how to run the Rubocop style checks. I highly suggest running those commands within the docker container as described in the section just above that one. |
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.
This looks good to me. Thanks for all the tests @brendandburns!
# in a multi-resource file had better be a valid k8s resource | ||
content = ::YAML.safe_load(f.content, aliases: true) | ||
likely_kubernetes_resource?(content) | ||
if f.type == "file" && f.name == "values.yaml" |
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.
Dependabot detects dockerfiles with names like Dockerfile.ci
or Dockerfile-test
by selecting files that contain /dockerfile/i
anywhere in the name.
From my experience it's common for Helm projects to do something similar with multiple values files, like values-staging.yaml
, and then helm install -f values-staging.yaml ...
. Not sure if we'll land on a filename-matching approach that will accommodate the majority of scenarios, but I wanted to bring it up for consideration.
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.
I'm ok to do this. Will add it in a later PR?
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.
Later PR is fine, esp because it will fail "loudly" so we won't forget about it... ie users will complain it's not working 😄
Rebased. |
@honeyankit this seems to be failing because of a permission problem unrelated to this PR. If it is something in this PR please let me know and I will try to fix it. |
Sorry for the confusion there. That's not a required check, so it's not a blocker for merging. The workflow in question pushes images built for PRs to GitHub Container Registry, so that we can easily deploy changes before merging. This step currently fails on PRs from forks, because forks won't have write access to GHCR (see #5668). This is a known issue and we're working on a solution. (Also, thanks for your contribution! I'm really excited to add support in Dependabot for Helm charts) |
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.
👍 in general, had a few trivial questions/suggestions...
} | ||
} | ||
] | ||
|
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.
Does helm generate these files without a final newline for some reason? I see this is true of several other files as well.
If not auto generated by Helm this way, can you please fix?
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.
Fixed.
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.
Thanks. I filed #5814 to add a linter for this to speed up the feedback loop.
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.
Actually, you mention fixed, but I still see missing newlines on this file and the other json file? Did you forget to git add
them perhaps?
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.
So the newlines were there, but I think that because the JSON wasn't fully formatted the newline detector was confused.
I think I have fixed this now.
Also, can you please squash the lint change commit into the original commit? We currently can't merge via squash due to a limitation in our release script so whatever the commit history is on this branch is what makes it into |
Comments addressed, commits squashed and rebased. Please take another look. Thanks! |
70e9e0c
to
a5152a6
Compare
Comment addressed. Please take another look. |
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.
LGTM. Thanks for the thorough test coverage!
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.
LGTM other than the missing newline...
Once you fix that, then we are probably ready to 🚢 given all the other ✅ on this PR. 😄
65a8ef3
to
6f6a533
Compare
newlines added. fwiw, the JSON formatter for VS Code/Codespaces removes the newline at the end of JSON files, so I'm not sure that newlines for JSON is standard, but nonetheless they should be there now. Please take another look. Thanks! |
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.
Thanks!
I have a few other PR's I need to deploy/merge first, but hopefully can push this one in the next day or two.
Oh interesting. I'm surprised by that, I'll have to research that a bit more. |
That looks better, @brendandburns see ☝️ I have no idea what went wrong, something in GitHub's I'll deploy this once CI goes green, and assuming everything looks good in production will merge this later tonight. |
@jeffwidman Thanks and yes, the @microsoft.com address is generally what I use. Great to see this merge! |
Hi @jeffwidman, Do we need to cut a new tag to use this feature? |
It is already deployed to GitHub prod, but requires a feature flag. If you are using it somewhere other than GitHub than you'll need to use |
I am using it outside of GH. When it'll be available on tagged release? |
@jeffwidman Any timeline on when the tag release will cut? |
We don't really do tagged releases anymore as our deployment model has changed a bit and we don't need them anymore, you can just pull the changes in from a sha |
Long term, I think we probably should still make tagged releases available
on RubyGems to make life easier for those who run Dependabot in custom
ways.
But since we are now deploying multiple times a day straight from PR
branches before they merge back to `main` we'll probably move away from
manually running the release scripts that push to RubyGems and instead
automate it to cut versions on a monthly or weekly basis. This also means
we'll likely need to move away from semver guarantees.
That said the above is my personal opinion and others on the team may feel
differently... So just overall there's some planning / internal discussion
that needs to happen and no one has had the bandwidth to drive that yet..
I'm hopeful I might have time before end of quarter but I can't guarantee
it.
|
Currently this is buried behind a feature flag for folks on github.com... We'd like to start slowly rolling it out for everyone, but before I do that, I'd like to test on a few repos to ensure no unexpected issues. I can / will setup a fake test repo, but there's nothing like live data to confirm things. Anyone watching this PR care to volunteer to have their repo(s) be part of the beta test? Repos must match the following criteria:
If interested, please comment on this thread with the URLs of any repos you own/maintain. |
@nilekhc This is now live on RubyGems: https://rubygems.org/gems/dependabot-omnibus/versions/0.213.0 |
Hey @jeffwidman, we can test with our repo if you are still looking for volunteers. Let me know what I need to do. cc @aramase |
Awesome thanks @nilekhc I will enable the feature flag on your repo tomorrow, and then since it's a public repo I think I can internally trigger a dry-run to test it. I'll post a follow-up once the flag is enabled. |
Also @nilekhc you'll need to enable a |
|
@jeffwidman It's working on our repo - |
Awesome news thanks for circling back! |
I've enabled the feature flag on 100% of github.com cloud repos, so anyone is now welcome to try this on their repos. If you run into any bugs, please file an issue. |
@jeffwidman Did you do any special setup while enabling this feature? I am trying to run it manually by building dependabot docker image and running dependabot/dependabot-core bundle exec ruby ./generic-update-script.rb but got error |
☝️ is resolved, right @nilekhc ? Catching up on old notifications... 😀 |
It is! |
This PR extends the recent Kubernetes support for Docker containers to common Helm chart formats:
or alternatively: