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(mix): implement lockFileMaintenance #33326

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

Conversation

sheerlox
Copy link
Contributor

@sheerlox sheerlox commented Dec 29, 2024

Changes

Adds support for the lockFileMaintenance option.

Refactors the lock file detection to differentiate umbrella projects from classic ones, as this implementation won't work on the former because they share a single mix.lock file for multiple mix.exs files.

Cleans up the getPkgReleases mocks by only returning the latest version, since these tests aren't about the tool version resolution.

Context

Resulting commit: sheerlox-repros/renovate-pr-33326@7ec9c87

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

@sheerlox sheerlox marked this pull request as draft December 29, 2024 02:02
@sheerlox sheerlox force-pushed the feat/manager-mix-lockfile-maintenance branch from d93a22e to d3b4ba4 Compare January 9, 2025 15:47
@sheerlox sheerlox marked this pull request as ready for review January 9, 2025 15:47
@sheerlox
Copy link
Contributor Author

Hello 👋 Is there anything I can do to help move this PR forward? No worries if it's a time issue, I totally get it. All the best!

@rarkins rarkins requested a review from viceice January 14, 2025 16:18
rarkins
rarkins previously approved these changes Jan 14, 2025
lib/modules/manager/mix/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.spec.ts Outdated Show resolved Hide resolved
@sheerlox
Copy link
Contributor Author

sheerlox commented Jan 14, 2025

Could one of you please explain why we should choose of of the following over the others, from a testing point of view?

I searched for a while when I did the PR and couldn't find an answer (this is why I initially left the removeDanglingContainers mocks as I didn't know what I was working with).

GlobalConfig.set({ ...adminConfig, binarySource: 'install' });
GlobalConfig.set({
  ...adminConfig,
  binarySource: 'docker',
  dockerSidecarImage: 'ghcr.io/containerbase/sidecar',
});

iiuc we can't use toMatchObject([]) without tying the test to the implementation details of the util/exec/docker module or mocking removeDanglingContainers (as it is the first command in the exec snapshot), but is there something else I should have in mind?

@sheerlox sheerlox requested review from rarkins and viceice January 14, 2025 18:55
@sheerlox
Copy link
Contributor Author

sheerlox/nodelix#28

Well, that's unexpected! The update-lockfile strategy wasn't working until right now, so I'm not 100% sure what caused this PR to be raised, but I'd say it's a (positive) side effect of #33325.

@sheerlox sheerlox requested a review from viceice January 17, 2025 16:12
rarkins
rarkins previously approved these changes Jan 22, 2025
@sheerlox
Copy link
Contributor Author

Hey there 👋 is this ready for merging or can I do anything else?

lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
@rarkins rarkins requested a review from viceice January 29, 2025 14:19
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

still open conversation

@sheerlox sheerlox requested a review from viceice January 30, 2025 12:07
lib/modules/manager/mix/artifacts.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/mix/artifacts.ts Outdated Show resolved Hide resolved
Comment on lines +66 to +68
logger.debug(
'Cannot use lockFileMaintenance in an umbrella project, see https://docs.renovatebot.com/modules/manager/mix/#lockFileMaintenance',
);
Copy link
Member

Choose a reason for hiding this comment

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

@rarkins should we use a different log level here? i'm unsure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind too much about debug logs

@sheerlox sheerlox requested a review from viceice February 2, 2025 09:56
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