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(bundler): add git refs support #32362

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

Conversation

Djiit
Copy link
Contributor

@Djiit Djiit commented Nov 6, 2024

Changes

Added git refs support for the bundler manager.

Context

Fixes #30990

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

@Djiit Djiit marked this pull request as ready for review November 6, 2024 10:29
@Djiit
Copy link
Contributor Author

Djiit commented Nov 15, 2024

Hey team, do you need anything from me to get this merged? Thank you 🙏 cc @rarkins

lib/modules/manager/bundler/extract.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/bundler/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/bundler/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/bundler/extract.ts Outdated Show resolved Hide resolved
lib/modules/manager/bundler/extract.ts Outdated Show resolved Hide resolved
rarkins
rarkins previously approved these changes Nov 21, 2024
@rarkins rarkins added this pull request to the merge queue Nov 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@rarkins
Copy link
Collaborator

rarkins commented Nov 21, 2024

Merged failed with this:

> tsc "--noEmit"

Error: lib/modules/manager/bundler/extract.ts(172,28): error TS23[39](https://github.com/renovatebot/renovate/actions/runs/11952139756/job/33317261723#step:4:40): Property 'registryUrl' does not exist on type 'string'.
Error: lib/modules/manager/bundler/extract.ts(173,45): error TS2339: Property 'registryUrl' does not exist on type 'string'.
Error: lib/modules/manager/bundler/extract.ts(176,28): error TS2339: Property 'sourceName' does not exist on type 'string'.
Error: lib/modules/manager/bundler/extract.ts(177,55): error TS2339: Property 'sourceName' does not exist on type 'string'.

Not sure why it was caught in the merge queue and not in PR.

@rarkins
Copy link
Collaborator

rarkins commented Nov 21, 2024

Now it failed here!

@rarkins
Copy link
Collaborator

rarkins commented Nov 23, 2024

There now seems to be a refactoring of the existing and non git refs logic too?

@Djiit
Copy link
Contributor Author

Djiit commented Nov 23, 2024

There now seems to be a refactoring of the existing and non git refs logic too?

Hey @rarkins , @viceice suggested I moved the regex at the module levels, so I did this for all the regexes here.
Other changes are also here to align practices, without any functional changes. Anything you'd like me to remove?

@Djiit
Copy link
Contributor Author

Djiit commented Nov 23, 2024

Also I'd need your help on the CI: I have not added any snapshots, just updated the existing ones (as some were already containing what was not yet parsed a gitrefs). How can I make this pass?

@rarkins
Copy link
Collaborator

rarkins commented Nov 24, 2024

I was referring to the changes under the "if (gemMatch)" logic branch. Is that logic meant to be the same as before? That logic is now all red and green in the diff.

@Djiit
Copy link
Contributor Author

Djiit commented Nov 24, 2024

Yes that's the same.

@rarkins
Copy link
Collaborator

rarkins commented Nov 25, 2024

Yes that's the same.

There's a change in logic regarding sourceName. Is it intentional or accidental?

@Djiit
Copy link
Contributor Author

Djiit commented Nov 25, 2024

Hu, that's my bad, I failed to put a comment about this. As registryUrl and sourceName are exclusive, I thought it was ok to transform the if into a else if. I reverted this change; it can be tackled in another PR not to interfer with the main change.
Thanks for your review.

@rarkins
Copy link
Collaborator

rarkins commented Nov 25, 2024

I think it was ok to make that change (I agree with you re: mutual exclusivity) but I wanted to check that it's intentional.

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.

Support git: dependencies for Bundler
3 participants