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

add Maven credential metadata to the URLs it searches for POM files #5884

Merged
merged 2 commits into from
Oct 17, 2022

Conversation

jakecoffman
Copy link
Member

Currently Dependabot uses any repository or pluginRepository URL it finds in the POM to search for parent POM files. However, there is no reason a child POM needs to have any repositories defined at all. It's fairly common for private/custom registries to only be defined in the parent which is published to the registry.

The reason why this works locally for Maven users is they often have a settings.xml which defines what registry to use, but that file isn't typically checked in to git.

Dependabot users commonly try to put the registry in dependabot.yml as a hint to where it can find the parent POM.

This PR takes that hint and puts Maven registries defined in dependabot.yml at the front of the list of registries to check.

@jakecoffman jakecoffman requested a review from a team as a code owner October 13, 2022 20:12
@jakecoffman
Copy link
Member Author

This should fix #5523

@jakecoffman jakecoffman force-pushed the jakecoffman/maven-creds-settings-xml branch from 0362877 to e562c98 Compare October 13, 2022 20:20
Copy link
Contributor

@honeyankit honeyankit left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@@ -155,6 +156,12 @@ def remote_pom_url(group_id, artifact_id, version, base_repo_url)
"#{artifact_id}-#{version}.pom"
end

def urls_from_credentials
@credentials.
select { |cred| cred["type"] == "maven_repository" }.
Copy link
Contributor

Choose a reason for hiding this comment

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

I had been thinking we'd only do this for creds with replaces-base: true. I can see now that you might want this but still allow a public registry though. Is there any potential for breaking existing jobs if we start to automatically move the registry sources to the front?

Alternatives might be:

  1. Introducing a new "global" key on the registry to indicate it comes from a global configuration not checked into the repo so we should insert it
  2. Use replaces-base which would also remove automatic usage of the default registry but allow the default registry to added back via inclusion in dependabot.yml registries.

But if it doesn't seem likely this would break anything we could try this out first.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thought was replaces-base would remove the default registry if it is true in any of the maven creds. I was going to follow this up by adding that in the next bite-sized PR.

This approach does allow fallback to Maven Central. For users of GitHub Registry that's probably what they want. It's also possible GHR users would want to check Central first to avoid 404ing first. If we drop the username and password requirement, then they have complete control of the order by specifying in dependabot.yml.

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.

None yet

3 participants