Skip to content
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

fix(datasource/custom): do not run digest update on version updates #29730

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

marcoesters
Copy link

@marcoesters marcoesters commented Jun 17, 2024

Changes

Add an exception for custom data sources to disallow digest updates when other updates are already in place.

Context

Relevant discussion: #28343

PR #28760 introduced the ability to perform digest updates, i.e., updating the digest without a version change. This had an unintended consequence under the following circumstances: if a version update also updates a digest, renovate will additionally try a digest update for the current version (not the updated version). If an API returns only the latest version (as shown in the k3s example), this will result in a misleading warning that a digest could not be found.

The following lines are the culprit:

    if (supportsDigests(config.datasource)) {
      if (config.currentDigest) {
        if (!config.digestOneAndOnly || !res.updates.length) {
          // digest update
          res.updates.push({
            updateType: 'digest',
            newValue: config.currentValue,
          });
        }

!res.updates.length is supposed to prevent such an extra update from happening. However, config.digestOneAndOnly is undefined for anything but Go and (sometimes) Bazel managers, so !config.digestOneAndOnly always evaluates to true.

I admit that this is a hack, but I don't have enough overview of the code base to replace digestOneAndOnly or find a way to change the condition without guaranteeing any fallout for other managers. This may need to be reevaluated comprehensively in the future.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

A test repository can be found here: https://github.com/marcoesters/renovate-miniconda-digest-reproducer

It contains the original update, but I added some simplified json files. Expected behavior would be:

  • major.json, minor.json, digest.json: update, no warnings
  • warning.json: no update, warning issued

Currently, renovate does the following:

  • digest.json: update, no warnings
  • major.json, minor.json: update, warning issued
  • warning.json: no update, warning issued

This PR results in the expected behavior

@rarkins
Copy link
Collaborator

rarkins commented Aug 21, 2024

@marcoesters thanks for this PR.

Could you:

  1. Add a test (which would have failed prior to this PR)
  2. Add a minimal reproduction repo demonstrating the new functionality?

@marcoesters
Copy link
Author

@marcoesters thanks for this PR.

Could you:

1. Add a test (which would have failed prior to this PR)

2. Add a minimal reproduction repo demonstrating the new functionality?

I will try and work on the test (typescript isn't a language I'm super familiar with). I simplified the repository I mentioned in my PR description to have simple data sources and covert the scenarios mentioned in the discussion: https://github.com/marcoesters/renovate-miniconda-digest-reproducer

Do I need to add it somewhere into the code base?

Comment on lines 546 to 551
if (
!(
config.digestOneAndOnly ?? config.datasource.startsWith('custom.')
) ||
!res.updates.length
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to convert this logic to an if/else to reduce the amount of negating. The if path can have logger.trace() inside it and the else then will contain the res.updates.push()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for future readability - the proposed change has definitely crossed a threshold for readability, maybe already had)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a few logic combinations and they are all very much unreadable, so I resorted to a boolean that will hopefully add some more to the semantics.

This is, unfortunately, still all very hacky, but I think a more sustainable solution will require deeper insights into the renovate code bases and use cases than I currently possess.

alwaysUpdateDigest = false;
} else if (config.datasource.startsWith('custom.')) {
// Custom datasources should not run a digest update
// on the current version if it already runs a version update.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it "already ran a version update" then wouldn't res.updates.length be non-zero length anyway?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, and !res.updates.length evaluates to false in this case. The problem in the original logic is config.digestOneAndOnly, which is true for gomod and undefined for other data sources.

So, for anything but gomod, the expression !config.digestOneAndOnly || !res.updates.length always evaluates to true no matter what res.updates.length is. So, renovate will try a "pure" digest update.

That pure digest update will look for the current version in the datasource. If the custom datasource only returns the latest version and that version is different from the current version, the digest update will fail and produce a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then this isn't the right solution, because it should be possible to do digest only updates with regex manager. I do that heavily on jenkinsfiles for docker images

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My solution here still allows for digest-only updates. My example repo covers scenarios for digest-only updates. The unit tests pass as well, which contain digest-only updates.

The problem was that renovate will do a digest-only additionally to a version + digest update for the same dependency. Under the circumstances above, this will result in an incorrect warning that renovate cannot look up a digest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants