-
Notifications
You must be signed in to change notification settings - Fork 994
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
port Docker updater improvements from Azure DevOps #8192
port Docker updater improvements from Azure DevOps #8192
Conversation
01298fc
to
817500f
Compare
Thank you so much for this! Would it be possible to have some tests to avoid regressions in the future please? |
@@ -9,8 +9,8 @@ | |||
WORDS_WITH_BUILD = /(?:(?:-[a-z]+)+-[0-9]+)+/ | |||
VERSION_REGEX = /v?(?<version>[0-9]+(?:\.[0-9]+)*(?:_[0-9]+|\.[a-z0-9]+|#{WORDS_WITH_BUILD}|-(?:kb)?[0-9]+)*)/i | |||
VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.\-]*)?$/i | |||
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-]*-)?#{VERSION_REGEX}$/i | |||
VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i | |||
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-_]*-)?#{VERSION_REGEX}$/i |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
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.
@abdulapopoola I'm not the greatest regex wizard but I think this would be a pre-existing condition (IE, this was not caused by our change to this REGEX but was already "inefficient" and just somehow grandfather-ed in). Do you have any advise for resolving this given that that's the case.
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.
As a team, I think we accept RE as necessary and that codeQL flagging might be too sensitive; tagging @deivid-rodriguez and @jakecoffman to get their thoughts.
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 think it's fine for CodeQL to flag these but I don't like it that it flags issues on any changed lines, even if they were present also before the changes. It places unnecessary burden on contributions like here.
Anyways, I believe it's fine to ignore.
Still, I had a look and I think this change may fix it, and it does not seem to break any specs:
diff --git a/docker/lib/dependabot/docker/tag.rb b/docker/lib/dependabot/docker/tag.rb
index 65c21e267..8eb805614 100644
--- a/docker/lib/dependabot/docker/tag.rb
+++ b/docker/lib/dependabot/docker/tag.rb
@@ -8,7 +8,7 @@ module Dependabot
class Tag
WORDS_WITH_BUILD = /(?:(?:-[a-z]+)+-[0-9]+)+/
VERSION_REGEX = /v?(?<version>[0-9]+(?:\.[0-9]+)*(?:_[0-9]+|\.[a-z0-9]+|#{WORDS_WITH_BUILD}|-(?:kb)?[0-9]+)*)/i
- VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.\-]*)?$/i
+ VERSION_WITH_SFX = /^#{VERSION_REGEX}(?<suffix>-[a-z][a-z0-9.]*)?$/i
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-]*-)?#{VERSION_REGEX}$/i
VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i
NAME_WITH_VERSION =
Just in case we want to change that separately.
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-]*-)?#{VERSION_REGEX}$/i | ||
VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i | ||
VERSION_WITH_PFX = /^(?<prefix>[a-z][a-z0-9.\-_]*-)?#{VERSION_REGEX}$/i | ||
VERSION_WITH_PFX_AND_SFX = /^(?<prefix>[a-z\-_]+-)?#{VERSION_REGEX}(?<suffix>-[a-z\-]+)?$/i |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
it "has the right details" do | ||
expect(dependency).to be_a(Dependabot::Dependency) | ||
expect(dependency.name).to eq("myreg/ubuntu") | ||
expect(dependency.version).to eq("17.04_") |
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.
Is it this a real example, or just a simplified version for the sake of testing? Do you have an example of a realworld image tagged like this?
So far we've been trying to support to most widely used versioning schemes out there since there's no official spec for versioning docker tags. But arbitrarily putting an underscore after the version number does not make a lot of sense to me.
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 is not a real example. The version we're thinking of is non-public, but it's a docker container for which the tag is shaped something like someRepo_19700101.4
. I can replace the above with that now that I know what shape we're imagining.
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, just to confirm, you need to support underscores as suffixes to the version, not as prefixes as originally stated, correct?
This should be ready for re-review. |
Would it be possible to split this PR into two commits, one for the helm charts feature, and another way for the detection of docker images with underscored suffixes? That'd help with the review! |
7de5383
to
4c02675
Compare
I have rebased the commits as per your request. |
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 is looking good. The changes for supporting underscore prefixes look good to me, feel free to propose separately if you want those merged straightaway. For heml charts I had some questions.
4c02675
to
c0a9b3a
Compare
@deivid-rodriguez I've cleaned up the commits based on your feedback and rebased on the current |
c0a9b3a
to
0f24892
Compare
0f24892
to
63fc248
Compare
63fc248
to
650e132
Compare
As part of the work inside Azure DevOps to improve dependabot (see #8179) there were some updates to the Docker updater that needed to be split out into their own PR, specifically:
(cc @brbayes-msft)