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(pub): Support updating lock files #19116

Merged
merged 10 commits into from Dec 24, 2022
Merged

feat(pub): Support updating lock files #19116

merged 10 commits into from Dec 24, 2022

Conversation

zeshuaro
Copy link
Contributor

@zeshuaro zeshuaro commented Nov 26, 2022

Changes

Add support to also update pubspec.lock when a Pub dependency is updated

Context

Documentation

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

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

I have verified these changes via:

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.

needs tests for install and docker mode. checkout cargo or nugget manager for samples

lib/modules/manager/pub/artifacts.spec.ts Outdated Show resolved Hide resolved
@zeshuaro
Copy link
Contributor Author

Docker mode tests

Hopefully I'm on the right direction for the docker mode tests. I tracked down that we probably need to update allToolConfig in containerbase and define the tools Dart and Flutter.

For Flutter, I think the config would be something like this, which seems to be working:

flutter: {
  datasource: 'flutter-version',
  depName: 'flutter',
  versioning: npmVersioningId,
},

But for Dart, we don't have a data source for it, so I think we would need to add a new data source for Dart. Potentially something similar to this issue: #12820. Would you be able to confirm? If this is the case, I'm also happy to work on adding a data source for Dart.

Install mode tests

For the install mode tests, I've updated the GlobalConfig but it's still not running in install mode (the exec snapshot doesn't contain the install-tool command). Is there some other config that I need to specify?

Test
it('supports install mode', async () => {
  GlobalConfig.set({ ...adminConfig, binarySource: 'install' });
  const execSnapshots = mockExecAll();
  fs.getSiblingFileName.mockReturnValueOnce('pubspec.lock');
  fs.readLocalFile.mockResolvedValueOnce('Old pubspec.lock');
  fs.readLocalFile.mockResolvedValueOnce('New pubspec.lock');
  expect(
    await pub.updateArtifacts({
      packageFileName: 'pubspec.yaml',
      updatedDeps: [{ depName: 'dep1' }],
      newPackageFileContent: 'sdk: flutter',
      config,
    })
  ).toEqual([
    {
      file: {
        type: 'addition',
        path: 'pubspec.lock',
        contents: 'New pubspec.lock',
      },
    },
  ]);
  expect(execSnapshots).toMatchObject([
    { cmd: 'install-tool flutter 3.3.9' },
    { cmd: 'flutter pub upgrade' },
  ]);
});

@rarkins
Copy link
Collaborator

rarkins commented Dec 2, 2022

Yes on allToolConfig. Consider semver versioning instead of npm if you don't need ranges, or keep as npm if you need npm's range syntax.

For Dart, you could maybe use https://github.com/dart-lang/sdk/tags although you want to make sure that it matches https://github.com/containerbase/base/blob/7c5021bf3f9c39e6513fe932becd52ee349057d8/src/usr/local/buildpack/tools/v2/dart.sh#L16

@zeshuaro
Copy link
Contributor Author

zeshuaro commented Dec 3, 2022

For Dart, you could maybe use https://github.com/dart-lang/sdk/tags although you want to make sure that it matches https://github.com/containerbase/base/blob/7c5021bf3f9c39e6513fe932becd52ee349057d8/src/usr/local/buildpack/tools/v2/dart.sh#L16

I remember Google often pushes the new tag first but then uploads the SDK to their storage site a few days later (this happened for Flutter at least). So it seems like a new data source for Dart would be preferable?

@rarkins
Copy link
Collaborator

rarkins commented Dec 3, 2022

Yes, it would be preferable

@zeshuaro zeshuaro mentioned this pull request Dec 6, 2022
6 tasks
@zeshuaro zeshuaro requested review from viceice and HonkingGoose and removed request for viceice and HonkingGoose December 21, 2022 10:14
@zeshuaro
Copy link
Contributor Author

Sorry if you received a couple of review re-requesting, GitHub doesn't let me to re-request both of you for some reasons.

@HonkingGoose
Copy link
Collaborator

Sorry if you received a couple of review re-requesting, GitHub doesn't let me to re-request both of you for some reasons.

There's a "refresh" icon you can use to re-request a review, see step 7 in the GitHub docs. Did that not work properly for you?

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review

HonkingGoose
HonkingGoose previously approved these changes Dec 21, 2022
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

I'm happy with the docs.

@zeshuaro
Copy link
Contributor Author

It did work, but it wouldn't let me to re-request reviews from both of you. So right now I requested one of you. If I request for the other person, GitHub would remove the review request from the first person 😓

lib/modules/manager/pub/artifacts.ts Outdated Show resolved Hide resolved
lib/modules/manager/pub/artifacts.spec.ts Outdated Show resolved Hide resolved
@rarkins rarkins merged commit b46e52c into renovatebot:main Dec 24, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 34.73.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@provokateurin
Copy link

Awesome work ❤️ Thanks a lot for taking over @rarkins !

@rarkins
Copy link
Collaborator

rarkins commented Dec 24, 2022

No, thanks to @zeshuaro !

@provokateurin
Copy link

Ooops I didn't check the Github UI correctly

@zeshuaro zeshuaro deleted the feature/pub-lockfile-maintenance branch December 25, 2022 00:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2023
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.

Renovate not updating pubspec.lock
6 participants