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

Use dependencies for yaml files from uap-core #32

Merged
merged 1 commit into from
Feb 16, 2020
Merged

Use dependencies for yaml files from uap-core #32

merged 1 commit into from
Feb 16, 2020

Conversation

douglaspalmer
Copy link
Contributor

We would like to use uap-java in the keycloak project (https://www.keycloak.org). However, to do so I need to make a change to your build system. Would my proposed changes be acceptable to you?

pom.xml Outdated
</configuration>
</execution>
</executions>
</plugin-->
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this all commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, commented it out to see if unpacking was necessary. It wasn't so I will delete it and update the pull request.

@bpossolo
Copy link
Contributor

bpossolo commented Oct 4, 2018

i think this change makes sense.
i always thought the <resource> blocks with references to {baseDir} kind of odd and unconventional for maven

@abstractj
Copy link

@bpossolo do you think we can get it in further releases? Is there anything missing?

@ThoSap
Copy link

ThoSap commented Feb 15, 2020

@bpossolo when will this be merged into master?
This would be awesome and very much appreciated by the community! 🚀🤩

@bpossolo
Copy link
Contributor

Ok. I’ll merge and just note that we should remove the git module reference to the other project

@bpossolo bpossolo merged commit 36e3062 into ua-parser:master Feb 16, 2020
@bpossolo
Copy link
Contributor

I just gave this PR a spin and it doesn't work.

There is no artifact in Maven central that matches:

  • group id: com.github.ua-parser
  • artifact id: uap-core

I presume this was never tested or you created the artifact yourself and installed it in your local Maven repo so I'm going to revert.

@douglaspalmer
Copy link
Contributor Author

The PR is well tested and works; you just need to apply the other half: ua-parser/uap-core#348

@bpossolo
Copy link
Contributor

Oh I see. It would have been good to link this PR to that one and made clear there was a dependency on that.

I don't have control of uap-core... so it will require that to be done first.

@bpossolo
Copy link
Contributor

Out of curiosity, can you explain what is preventing you from using the uap-java artifact directly which includes the regexp yaml files?

@douglaspalmer
Copy link
Contributor Author

Out of curiosity, can you explain what is preventing you from using the uap-java artifact directly which includes the regexp yaml files?

For my use case; we build everything from source internally. We don't use external binaries.

@bpossolo
Copy link
Contributor

Ok, I thought that might be that case.
Are you aware that the Parser class has a constructor which accepts an InputStream to the regexp file?
In the meantime, can you get the uap-core repo + it's yaml file then use that constructor instead?

I presume the maven tests will all fail without uap-core where it's expected to be so you might need to add -Dmaven.test.skip=true when building the artfiact.

@bpossolo
Copy link
Contributor

I commented in the other PR asking @commenthol for advice.
I can't just merge your PR into his project as I don't really consider myself a maintainer for that.

If I remember correctly, I'll need to go through all the steps to get a new artifact published to Maven Central which requires some time as it's a bit of a manual back-and-forth process.

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.

4 participants