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.

Sign up for GitHub

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

Jump to bottom

fix(maven): Use correct snapshot URL in getDependencyInfo #15380

Merged
merged 7 commits into from May 13, 2022
Merged

fix(maven): Use correct snapshot URL in getDependencyInfo #15380

merged 7 commits into from May 13, 2022

Conversation

candrews
Copy link
Contributor

Changes

Move createUrlForDependencyPom from index.ts to util.ts
Use the moved createUrlForDependencyPom in getDependencyInfo

Context

Fixes incorrect URL access for snapshots identified at #15227 (comment)

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

@candrews candrews force-pushed the maven-getDependencyInfo-snapshots branch from 3764604 to 657c68a Compare April 29, 2022 18:19
@candrews candrews mentioned this pull request Apr 29, 2022
Closed
viceice
viceice requested changes Apr 29, 2022
View reviewed changes
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.

please create a separate PR for the refactoring, so we can validate/ review changes more easily

@candrews candrews mentioned this pull request Apr 29, 2022
Merged
6 tasks
@candrews candrews force-pushed the maven-getDependencyInfo-snapshots branch from 657c68a to dd1c8b2 Compare April 29, 2022 19:21
@candrews
Copy link
Contributor Author

please create a separate PR for the refactoring, so we can validate/ review changes more easily

I created the new PR that just does refactor (no functional changes): #15381

This PR now has the commit from that PR as well as a second commit that's just the functional change.

@candrews candrews force-pushed the maven-getDependencyInfo-snapshots branch from dd1c8b2 to 5e5d95a Compare May 2, 2022 13:26
@candrews
Copy link
Contributor Author

candrews commented May 9, 2022

@viceice (or anyone else appropriate) can you please review/merge this PR?

@viceice
Copy link
Member

viceice commented May 9, 2022

please read our contribution guide

https://github.com/renovatebot/renovate/blob/main/.github/contributing.md#re-requesting-a-review

@candrews candrews requested a review from viceice May 9, 2022 20:58
viceice
viceice reviewed May 10, 2022
View reviewed changes
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 think this should have new unit tests too

rarkins
rarkins requested changes May 10, 2022
View reviewed changes
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.

Is it possible to add a test for this? (which failed before this change, passes after it)

@candrews
Copy link
Contributor Author

Is it possible to add a test for this? (which failed before this change, passes after it)

I've added such a test.

@candrews candrews requested review from rarkins and viceice May 12, 2022 22:16
@candrews candrews force-pushed the maven-getDependencyInfo-snapshots branch from 8c6fd5c to 95eb4cb Compare May 13, 2022 00:22
rarkins
rarkins previously approved these changes May 13, 2022
View reviewed changes
@rarkins rarkins requested a review from zharinov May 13, 2022 06:16
@candrews candrews requested a review from viceice May 13, 2022 12:35
@candrews
Copy link
Contributor Author

I don't know why this test is failing: https://github.com/renovatebot/renovate/runs/6423225547?check_suite_focus=true

I also don't see how that could be connected to any changes made in this MR.

viceice
viceice approved these changes May 13, 2022
View reviewed changes
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.

🤷‍♂️

@viceice viceice requested a review from zharinov May 13, 2022 15:15
@rarkins rarkins merged commit ae4a574 into renovatebot:main May 13, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 32.52.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zharinov zharinov zharinov approved these changes

@viceice viceice viceice approved these changes

@rarkins rarkins rarkins left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@candrews @viceice @renovate-release @zharinov @rarkins