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

feat(datasource/custom): Support digest in custom datasource #26299

Closed

Conversation

fstehle
Copy link
Contributor

@fstehle fstehle commented Dec 14, 2023

Changes

To support digests a datasource has to implement the getDigest method.
This PR implements the method returning a null promise.
The value itself can be provided in the getReleases method.

Context

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

@fstehle fstehle changed the title Support digest in custom datasource fix(datasource/custom): Support digest in custom datasource Dec 14, 2023
@fstehle fstehle force-pushed the custom_datasource_digest_support branch from c470002 to dfc34f7 Compare December 14, 2023 16:34
To support digests a datasource has to implement the getDigest method.
The value itself can be provided in the getReleases method.
@fstehle fstehle force-pushed the custom_datasource_digest_support branch from dfc34f7 to 08edb90 Compare December 14, 2023 16:34
@fstehle fstehle marked this pull request as ready for review December 14, 2023 16:34
@rarkins rarkins requested a review from secustor December 14, 2023 16:40
Copy link
Collaborator

@secustor secustor left a comment

Choose a reason for hiding this comment

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

Not fond of this solution, tough a better fix would need bigger changes.

lib/workers/repository/process/lookup/index.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
@viceice
Copy link
Member

viceice commented Dec 15, 2023

why do we need this? that function shouldn't be called when the digest is already returned in releases.

@fstehle
Copy link
Contributor Author

fstehle commented Dec 15, 2023

Not fond of this solution, tough a better fix would need bigger changes.

Yes, absolutely. Would you suggest to modify supportsDigests in

export function supportsDigests(datasource: string | undefined): boolean {
instead?

@fstehle
Copy link
Contributor Author

fstehle commented Dec 15, 2023

why do we need this? that function shouldn't be called when the digest is already returned in releases.

That is not what I observed, supportsDigests returns false and no digest update is made.

See the following lines for that:

  • export function supportsDigests(datasource: string | undefined): boolean {
    )
  • if (supportsDigests(config.datasource)) {
    if (config.currentDigest) {
    if (!config.digestOneAndOnly || !res.updates.length) {
    // digest update
    res.updates.push({
    updateType: 'digest',
    newValue: compareValue,
    });
    }
    } else if (config.pinDigests) {
    // Create a pin only if one doesn't already exists
    if (!res.updates.some((update) => update.updateType === 'pin')) {
    // pin digest
    res.updates.push({
    isPinDigest: true,
    updateType: 'pinDigest',
    // TODO #22198
    newValue: config.currentValue!,
    });
    }
    }
    if (versioning.valueToVersion) {
    // TODO #22198
    res.currentVersion = versioning.valueToVersion(res.currentVersion!);
    for (const update of res.updates || /* istanbul ignore next*/ []) {
    // TODO #22198
    update.newVersion = versioning.valueToVersion(update.newVersion!);
    }
    }
    if (res.registryUrl) {
    config.registryUrls = [res.registryUrl];
    }
    // massage versionCompatibility
    if (
    is.string(config.currentValue) &&
    is.string(compareValue) &&
    is.string(config.versionCompatibility)
    ) {
    for (const update of res.updates) {
    logger.debug({ update });
    if (is.string(config.currentValue) && is.string(update.newValue)) {
    update.newValue = config.currentValue.replace(
    compareValue,
    update.newValue,
    );
    }
    }
    }
    // update digest for all
    for (const update of res.updates) {
    if (config.pinDigests === true || config.currentDigest) {
    // TODO #22198
    update.newDigest ??=
    dependency?.releases.find((r) => r.version === update.newValue)
    ?.newDigest ?? (await getDigest(config, update.newValue))!;
    // If the digest could not be determined, report this as otherwise the
    // update will be omitted later on without notice.
    if (update.newDigest === null) {
    logger.debug(
    {
    packageName: config.packageName,
    currentValue: config.currentValue,
    datasource: config.datasource,
    newValue: update.newValue,
    bucket: update.bucket,
    },
    'Could not determine new digest for update.',
    );
    // Only report a warning if there is a current digest.
    // Context: https://github.com/renovatebot/renovate/pull/20175#discussion_r1102615059.
    if (config.currentDigest) {
    res.warnings.push({
    message: `Could not determine new digest for update (${config.datasource} package ${config.packageName})`,
    topic: config.packageName,
    });
    }
    }
    } else {
    delete update.newDigest;
    }
    if (update.newVersion) {
    const registryUrl = dependency?.releases?.find(
    (release) => release.version === update.newVersion,
    )?.registryUrl;
    if (registryUrl && registryUrl !== res.registryUrl) {
    update.registryUrl = registryUrl;
    }
    }
    }
    }
    if (res.updates.length) {
    delete res.skipReason;
    }
    // Strip out any non-changed ones
    res.updates = res.updates
    .filter(
    (update) => update.newValue !== null || config.currentValue === null,
    )
    .filter((update) => update.newDigest !== null)
    .filter(
    (update) =>
    (is.string(update.newName) &&
    update.newName !== config.packageName) ||
    update.isReplacement === true ||
    update.newValue !== config.currentValue ||
    update.isLockfileUpdate === true ||
    // TODO #22198
    (update.newDigest &&
    !update.newDigest.startsWith(config.currentDigest!)),
    );
    // If range strategy specified in config is 'in-range-only', also strip out updates where currentValue !== newValue
    if (config.rangeStrategy === 'in-range-only') {
    res.updates = res.updates.filter(
    (update) => update.newValue === config.currentValue,
    );
    }
    // Handle a weird edge case involving followTag and fallbacks
    if (config.rollbackPrs && config.followTag) {
    res.updates = res.updates.filter(
    (update) =>
    res.updates.length === 1 ||
    /* istanbul ignore next */ update.updateType !== 'rollback',
    );
    }
    } catch (err) /* istanbul ignore next */ {

@fstehle fstehle requested a review from secustor December 15, 2023 08:23
lib/modules/datasource/custom/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

The snapshot is with the latest change obsolete.
You can remove it with pnpm run jest -u

@secustor
Copy link
Collaborator

Not fond of this solution, tough a better fix would need bigger changes.

Yes, absolutely. Would you suggest to modify supportsDigests in

export function supportsDigests(datasource: string | undefined): boolean {

instead?

The logic in lookup/index.ts would need to modified, which needs extensive testing on real repos.

travisshivers added a commit to barney-ci/renovate that referenced this pull request Feb 23, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
travisshivers added a commit to barney-ci/renovate that referenced this pull request Feb 26, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
travisshivers added a commit to barney-ci/renovate that referenced this pull request Mar 1, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
travisshivers added a commit to barney-ci/renovate that referenced this pull request Mar 8, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Needs deconflicting and continuing

travisshivers added a commit to barney-ci/renovate that referenced this pull request Mar 26, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
travisshivers added a commit to barney-ci/renovate that referenced this pull request Apr 17, 2024
Support digets for custom datasource.
The getDigest method needs to exist on the custom datasource for
renovate to use the digest provided from the main versions/releases
endpoint. If the versions response always includes the digest, the
getDigest method doesn't actually need to be implemented.

https://github.com/renovatebot/renovate/blob/14b67888307e2a83219e4bb90f7f0f71f5e608bd/lib/workers/repository/process/lookup/index.ts#L446

See renovatebot#26299
@acortelyou
Copy link
Contributor

This PR would resolve request for help discussion #28343.

I hope we can find a way to get this code moving again.

It looks like the suggestions from the review were just nits: if @fstehle doesn't have bandwidth to revisit, perhaps the maintainers can update the PR with their fixes or an interested party (myself) could fork the PR.

@rarkins
Copy link
Collaborator

rarkins commented Apr 30, 2024

The latter

@fstehle
Copy link
Contributor Author

fstehle commented Apr 30, 2024

Currently quite busy, would be great if you could create a fork PR

rarkins and others added 3 commits April 30, 2024 12:30
Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
# Conflicts:
#	lib/workers/repository/process/lookup/index.spec.ts
@rarkins rarkins requested a review from secustor April 30, 2024 10:35
Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Fixed conflicts, but now I defer to @secustor

newValue?: string,
): Promise<string | null> {
// Return null here to support setting a digest: value can be provided digest in getReleases
// istanbul ignore next
Copy link
Member

Choose a reason for hiding this comment

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

should be removed, it seems there's a test for it

This comment was marked as outdated.

@viceice viceice changed the title fix(datasource/custom): Support digest in custom datasource feat(datasource/custom): Support digest in custom datasource Apr 30, 2024
Copy link
Collaborator

@secustor secustor left a comment

Choose a reason for hiding this comment

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

Needs test fix and removal of ignore statement, else LGTM

@acortelyou
Copy link
Contributor

See linked PR for commits to address outstanding items.

Either cherry-pick the commits on to here (i don't have permissions) or publish that one and merge from there.

Thanks!

Comment on lines +3871 to +3880
const res = await lookup.lookupUpdates(config);
expect(res).toMatchObject({
updates: [
{
newDigest: '0123456789abcdef',
newValue: '1.0.0',
updateType: 'digest',
},
],
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const res = await lookup.lookupUpdates(config);
expect(res).toMatchObject({
updates: [
{
newDigest: '0123456789abcdef',
newValue: '1.0.0',
updateType: 'digest',
},
],
});
const { updates } = await Result.wrap(
lookup.lookupUpdates(config),
).unwrapOrThrow();
expect(updates).toEqual([
{
newDigest: '0123456789abcdef',
newValue: '1.0.0',
updateType: 'digest',
},
]);

newValue?: string,
): Promise<string | null> {
// Return null here to support setting a digest: value can be provided digest in getReleases
// istanbul ignore next

This comment was marked as outdated.

newValue?: string,
): Promise<string | null> {
// Return null here to support setting a digest: value can be provided digest in getReleases
// istanbul ignore next
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// istanbul ignore next

@ivankatliarchuk
Copy link
Contributor

Does anyone know whether or not these changes will support Gitlab MD5 https://gitlab-docker-machine-downloads.s3.amazonaws.com/v0.16.2-gitlab.25/index.html?

@secustor
Copy link
Collaborator

secustor commented May 1, 2024

Does anyone know whether or not these changes will support Gitlab MD5 https://gitlab-docker-machine-downloads.s3.amazonaws.com/v0.16.2-gitlab.25/index.html?

No.

Closing as this has been implemented in the linked PR.

@secustor secustor closed this May 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants