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 support for updating dependencies in target files #6913

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

Conversation

laeubi
Copy link

@laeubi laeubi commented Mar 25, 2023

The eclipse-pde target files (extension .target, content is xml) supports for a while to also mention maven dependencies.

This enhances the file fetcher to scan for target files in a repository and the file parser to parse any maven type location.

Fix #4682


I'm creating this as a draft as it is missing a test case, this is my first time using ruby, so any help / hints would be appreciated.

@deivid-rodriguez
Copy link
Contributor

Hi @laeubi, thanks for working on this. Regarding test cases, I recommend you inspire on existing test case. For example, you can change any of the existing selectors to something incorrect, and see which tests fail (hopefully, otherwise those selectors would be uncovered by tests... 😅). That should give you the relevant tests covering the implementation of those selectors that you can use as an inspiration.

@mjmeijer
Copy link

Unfortunately Github doesn't have #likes so I just post this comment as token of appreciation

@yeikel
Copy link
Contributor

yeikel commented Apr 14, 2023

Unfortunately Github doesn't have #likes so I just post this comment as token of appreciation

You have reactions and that's pretty close 😄

@laeubi laeubi marked this pull request as ready for review June 6, 2023 04:56
@laeubi laeubi requested a review from a team as a code owner June 6, 2023 04:56
@github-actions github-actions bot added the L: java:maven Maven packages via Maven label Jun 6, 2023
@laeubi
Copy link
Author

laeubi commented Jun 6, 2023

I rebased now on current main branch and set this to "Ready For Review", I have also tried to adapt one of the test-cases to target files, would be good to get a feedback from a maintainer if I'm on the right track.

@jurre
Copy link
Member

jurre commented Jun 6, 2023

Thanks for working on this! Overall I think the PR looks good, but I don't know much about eclipse target files and their format, do you have any docs that you'd recommend so I can read up on it a bit?

@laeubi
Copy link
Author

laeubi commented Jun 6, 2023

@jurre first, feel free to ask any question about the format I can provide examples/insights if needed (I'm just a total newbie on ruby what makes it quite hard to adjust things).

There is some official documentation here:
https://help.eclipse.org/latest/index.jsp?topic=/org.eclipse.pde.doc.user/concepts/target.htm

but it is all a bit generic as usually users use the UI and do not really fiddle around with the details of serialization format, but it is also quite simple:

  1. File format is XML
  2. Root element is <target> that contains among others an element <locations> that has 0..n elements named <location>, each location has a type attribute, where only the type Maven is relevant here, a quite complex example can be found here: https://github.com/eclipse-m2e/m2e-core/blob/master/target-platform/target-platform.target, while the PR contains one reduced to the bare minimum: https://github.com/dependabot/dependabot-core/blob/54918367133820e94fbae4a332ae51d335e874ac/maven/spec/fixtures/target-files/example.target
  3. As one can see, a target file might contain other things (like pom.xml), but for this purpose one can completely ignore that, the only relevant part is what an XPath of dependencies > dependency would select

@jurre
Copy link
Member

jurre commented Jun 6, 2023

Thanks! I notice the Maven tests and the linter are failing, would you mind taking a look at those?

@laeubi
Copy link
Author

laeubi commented Jun 9, 2023

Thanks! I notice the Maven tests and the linter are failing, would you mind taking a look at those?

I'll try to look at it, but as said my ruby skills are quite limited so I first need to understand how all these works, if someone mor familiar with rubby could take a look and maybe give some hints it would be helpfull!

@laeubi
Copy link
Author

laeubi commented Jul 6, 2023

Thanks! I notice the Maven tests and the linter are failing, would you mind taking a look at those?

@jurre I now was able to fix the issue with the test and also other checks are all green now so I think it would be ready for review.

@laeubi
Copy link
Author

laeubi commented Jul 17, 2023

@jurre what would be the next steps for this to become merged?

@jurre
Copy link
Member

jurre commented Jul 17, 2023

@laeubi someone on my team will deploy the changes to production, monitor to ensure they don't introduce regressions and then merge the changes in

@HannesWell
Copy link

@laeubi someone on my team will deploy the changes to production, monitor to ensure they don't introduce regressions and then merge the changes in

Is there an update how it is working? :)

@jurre
Copy link
Member

jurre commented Aug 17, 2023

These changes have not been deployed yet, so no update available, hoping someone will get to it soon 👍

Christoph Läubrich added 3 commits August 23, 2023 18:37
The eclipse-pde target files (extension .target, content is xml)
supports for a while to also mention maven dependencies.

This enhances the file fetcher to scan for target files in a repository
and the file parser to parse any maven type location.

Fix dependabot#4682
@HannesWell
Copy link

These changes have not been deployed yet, so no update available, hoping someone will get to it soon 👍

Are there any updates yet? :)

@HannesWell
Copy link

These changes have not been deployed yet, so no update available, hoping someone will get to it soon 👍

Can you give us any update on this? It would be really nice to have this. :)

@tivervac
Copy link

@jurre Can this be deployed now? Anything we can help with?

@abdulapopoola
Copy link
Member

@tivervac , if you could help with resolving the conflicts, we can take a stab at getting it merged. Thanks.

@HannesWell
Copy link

@tivervac , if you could help with resolving the conflicts, we can take a stab at getting it merged. Thanks.

@laeubi is probably the right one to resolve the conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: java:maven Maven packages via Maven
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for maven-dependecies inside eclipse-pde target files
8 participants