docker: consider the tag when checking if a digest is up-to-date#13794
docker: consider the tag when checking if a digest is up-to-date#13794kbukum1 merged 1 commit intodependabot:mainfrom
docker: consider the tag when checking if a digest is up-to-date#13794Conversation
df3420b to
37c279c
Compare
|
@kbukum1 Any chance you can find me a reviewer for this change? |
|
Seems like the change is causing an issue. I am taking approval back. Can you trace it according to the following error? Also, I believe we need a proper spec to catch this if possible. It looks like the variable being used in A spec for this scenario would help catch this earlier. Thanks! |
|
@yeikel |
kbukum1
left a comment
There was a problem hiding this comment.
Need to fix the data type error. Also we may need to add a spec to catch the wrong data type issue
db8dbbd to
192d9a8
Compare
Thank you for the review and for sharing the details. The exception makes sense (and it’s unfortunate), but I’m a bit confused given the scope of my change. Are you confident this is caused by my change, rather than a separate, pre-existing production issue that just surfaced now? I ask because, based on the call stack, the method I updated doesn’t appear to be involved in the failing execution path. Specifically, I reviewed the execution path of my new method call I'd be happy to help investigate further but it honestly seems unrelated. Is this problem in the logs without my changes? Do you have access to the logs, the input payload, or a reproducible example that triggers the error? |
615e194 to
2ed382f
Compare
@yeikel , |
@yeikel , Sorry for troubling you. Yes the error was related to an existing error but because line numbers are changes it was showing as new error on Sentry. I already created a fix PR for it and after merging fix, I will also merge this PR. |
No problem. Thank you for taking the time to confirm |
3d10a68 to
13badc1
Compare
13badc1 to
4f8f9e9
Compare
|
Thanks @yeikel fixing the conflict. I was also trying to do it here. I will try to deploy that soon. Hopefully we will not have any issue. |
Fixed the problem in th following PR, https://github.com/github/github/pull/411983
Thank you for merging it. Please ping me if you find any issues as a result of this change |
|
@yeikel, I'm not sure but I suspect this change is causing docker updates to no longer work. Our updates in github.com/github/dependabot-action now return |
Thanks for the message. I'll investigate with high priority and get back today |
|
@yeikel, see below for my findings. In the following code you may want to check CC: @pavera Relevant code: sig { returns(T::Boolean) }
def digest_up_to_date?
return true unless updated_digest
digest_requirements.all? do |req|
source = req.fetch(:source)
source_digest = source.fetch(:digest)
source_tag = source[:tag]
expected_digest =
if source_tag
digest_of(source_tag)
else
updated_digest
end
# If we can't determine the expected digest (e.g., due to transient registry errors),
# we can't prove the digest is out of date, so conservatively assume it's up to date.
# This prevents false positives where we incorrectly mark dependencies as needing updates.
next true if expected_digest.nil?
source_digest == expected_digest
end
end |
|
@yeikel , Just let you know. I am reverting this for now. When we have complete solution with the fix, we can deploy a new PR. Also please note that, it will be great also to have a spec with the fix to catch this so we can be sure we are not going to have issue. Let me know if you are not available for this, so that I can check it later in my availability. CC: @pavera |
|
Apologies for the noise. I'll check and confirm and get back with more tests |
|
For what is worth, I do not see any registry failures in the job: https://github.com/github/dependabot-action/actions/runs/20377428237/job/58559469679 So it is unclear if that's the actual reason. In any case, I'll investigate and confirm |
What are you trying to accomplish?
When verifying whether Docker image digests are up to date, we previously compared every requirement’s
source.digestagainstupdated_digest. This was incorrect for requirements that include asource.tag, as the expected digest should be derived from that tag.Fixes #11215
How will you know you've accomplished your goal?
Before:
After:
Checklist