Bump airbase#67
Conversation
shangm2
force-pushed
the
bumpAirbase
branch
21 times, most recently
from
52a8ea7 to
e2650c2
Compare
| <version>${dep.airlift.version}</version> | ||
| </dependency> | ||
|
|
||
| <dependency> |
There was a problem hiding this comment.
why was this dependency removed. isn't it still needed?
There was a problem hiding this comment.
@rschlussel there is a duplicate in the pom already. No idea why it worked before.
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-dependency-plugin</artifactId> | ||
| <version>3.8.1</version> | ||
| <configuration> | ||
| <skip>true</skip> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
Why are we skipping this? It provides protection against runtime issues due to conflicting dependencies/classes on the classpath. I can understand the modernizer being a nuisance, but the dependency plugin is one we should not be disabling.
There was a problem hiding this comment.
@ZacBlanco Glad you are back. I don't think the errors from those plugins is related to our change. I just need a manual cut as you did before. I am ok not to merge this pr.
There was a problem hiding this comment.
Can you share what conflicts you were seeing? Just want to be sure the version upgrade doesn't cause dependency conflicts.
There was a problem hiding this comment.
Sure. Let me remove the skipping.
There was a problem hiding this comment.
The error message is
[INFO] --- dependency:3.8.1:analyze-only (default) @ drift-codec-utils --- Error: Non-test scoped test only dependencies found: Error: com.facebook.drift:drift-api:jar:1.44-SNAPSHOT:compile
which is false since compile scope is needed. @rschlussel @ZacBlanco
There was a problem hiding this comment.
if you add test does it fix it, or complain about the drift-api being a required compile-time dependency?
For those cases, you need to manually ignore that specific dependency in the configuration
There was a problem hiding this comment.
it is the other way around I think. It thinks compile is not necessary but it is needed transitively.
There was a problem hiding this comment.
I guess let me pull in those fix you guys did in the new repo. https://github.com/prestodb/airlift/blob/master/drift/drift-codec-utils/pom.xml I was trying to avoid duplicate work.
|
@ZacBlanco Let me know if you can make a manual cut without the need of this pr as before. More discussion is on slack and I have tagged you there. |
|
@shangm2 You might have pinged the wrong account (had to make a new slack account, old one was tied to my IBM email). I won't be able to cut a release for you. I would start a thread in the #dev channel if you need to cut a release for the drift repository. |
I see. Yeah, I already did a thread. |
ZacBlanco
left a comment
ZacBlanco
left a comment
There was a problem hiding this comment.
Not really approving of these changes, but for a temporary release I am ok with this given we're eventually migrating to the drift in the airlift repo.
|
@rschlussel @ZacBlanco I pulled in changes from the new repo and the pr is green now. Can I have another approval? Thanks |
4 participants
Uh oh!
There was an error while loading. Please reload this page.