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(ruby): extract version with quotation #17222

Merged
merged 5 commits into from Aug 19, 2022

Conversation

qnighy
Copy link
Contributor

@qnighy qnighy commented Aug 17, 2022

Changes

This is an attempt to fix a problem introduced in #17091.

In the bump strategy, the Ruby versioning sometimes tries to add a >= constraint to the existing ~> constraint, but it fails.

I guess Renovate is failing in the following way:

Ruby versioning module preserves quotations:

  • ~> 2.3 may be updated to ~> 3.0 or ~> 3.0, >= 3.0.1
  • '~> 2.3' may be updated to '~> 3.0' or '~> 3.0', '>= 3.0.1'
  • >= 1.0, < 2.0 may be updated to >= 2.0, < 3.0
  • '>= 1.0', '< 2.0' may be updated to '>= 2.0', '< 3.0'

Bundler manager retains quotations only if there are multiple constraints:

  • From gem 'rails', '~> 6.1', ~> 6.1 will be extracted.
  • From gem 'rails', '~> 6.1', '>= 6.1.2', '~> 6.1', '>= 6.1.2' will be extracted.

Therefore, if there was only one constraint, it will replace the content in the quotation. If the versioning module decides to add another constraint, it will be added within the quotation:

# Before
gem 'rails', '~> 6.1'
# After
gem 'rails', '~> 7.0, >= 7.0.2'
# Expected
gem 'rails', '~> 7.0', '>= 7.0.2'

This is not what Bundler expects and the update fails.

This PR tries to fix the issue by changing the Bundler's side by always including quotation marks when extracting versions, but I'm not sure this is an optimal solution. Other possible solutions are:

  • Try to modify the source code after changing the version in Gemfile.
  • Change the ruby versioning module so that quotation marks are necessary.
  • Pass certain metadata from the Bundler manager to the ruby versioning module to request quotation marks.
  • Separate the ruby versioning module into two: one that supports only a single ruby range and one that supports multiple quoted ranges.

Per comment #17222 (comment) , it is now Bundler manager's job to add/remove quotes in appropriate places. This is done by adding updateDependency function to the manager.

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 tick 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

@qnighy qnighy requested a review from viceice August 17, 2022 10:39
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.

I've a simpler solution

lib/modules/manager/bundler/update.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Aug 17, 2022

I've looked again in code and it seems we should always extract with quotes, so that the folling will also add them

if (newValue && regEx(/^('|")/).exec(currentValue)) {
const delimiter = currentValue[0];
return newValue
.split(',')
.map((element) =>
element.replace(
regEx(`^(?<whitespace>\\s*)`),
`$<whitespace>${delimiter}`
)
)
.map(
(element) =>
element.replace(/(?<whitespace>\s*)$/, `${delimiter}$<whitespace>`) // TODO #12875 adds ' at front when re2 is used
)
.join(',');
}

@qnighy
Copy link
Contributor Author

qnighy commented Aug 17, 2022

I've looked again in code and it seems we should always extract with quotes, so that the folling will also add them

That is precisely what the original commit 347d4a1 intends to do. So I'm going to revert the later commits.

@qnighy qnighy requested a review from viceice August 17, 2022 14:56
@qnighy
Copy link
Contributor Author

qnighy commented Aug 19, 2022

Reverted to the original commit, but there is one thing to note:

If we let the versioning module handle quotes, the quotes appear in the PR title:

image

Otherwise, if we handle quotes in the manager, the quotes are not shown.

image

I do not mind the quotes appearing in the title, but I'm willing to revert back to the other implementation again if the quotes are undesirable.

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.

handling in versioning is the easier solution for now

@viceice viceice enabled auto-merge (squash) August 19, 2022 09:50
@viceice viceice merged commit 633de4f into renovatebot:main Aug 19, 2022
@qnighy qnighy deleted the fix/bundler-keep-quote branch August 19, 2022 10:15
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.165.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
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.

None yet

3 participants