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

Correct typo in artifactId of dependency in bom pom.xml #6989

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@normanmaurer
Member

normanmaurer commented Jul 18, 2017

Motivation:

There was a typo in a dependency in the bom pom.xml which lead to have it specify a non-existing artifact and also so not have the maven release plugin update the version correctly.

Modifications:

Rename netty-transport-unix-common to netty-transport-native-unix-common and also fix the version.

Result:

Fixes [#6979]
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 18, 2017

Member

@tsachev and @wilkinsona PTAL ... This should fix [#6979]

Member

normanmaurer commented Jul 18, 2017

@tsachev and @wilkinsona PTAL ... This should fix [#6979]

@Scottmitch

lgtm

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 19, 2017

Member

I thought we had this at one point though ... and there was something with the release process that prevented using the variable? I may be confusing some other issue though...

Member

Scottmitch commented Jul 19, 2017

I thought we had this at one point though ... and there was something with the release process that prevented using the variable? I may be confusing some other issue though...

@wilkinsona

This comment has been minimized.

Show comment
Hide comment
@wilkinsona

wilkinsona Jul 19, 2017

Contributor

That was my concern too. It looks fine in terms of using the variable consistently, but I don't know anything about Netty's release process so I also don't know if it'll work as intended.

Contributor

wilkinsona commented Jul 19, 2017

That was my concern too. It looks fine in terms of using the variable consistently, but I don't know anything about Netty's release process so I also don't know if it'll work as intended.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 19, 2017

Member

@Scottmitch @wilkinsona the problem is that if we use the version directly the maven release plugin will NOT update the version for the dependencies that also have the classifier listed. So I think this is the way to go and also "consistent" with how we add the dependencies in the all module.

Member

normanmaurer commented Jul 19, 2017

@Scottmitch @wilkinsona the problem is that if we use the version directly the maven release plugin will NOT update the version for the dependencies that also have the classifier listed. So I think this is the way to go and also "consistent" with how we add the dependencies in the all module.

@tsachev

This comment has been minimized.

Show comment
Hide comment
@tsachev

tsachev Jul 19, 2017

I do not have an opinion on using project.version (unless it makes the bom useless as parent, but that is not its purpose anyway).
There might be something in the release process that replaces ${project.version} with the actual project version I don't know.

But #6979 is about using wrong artifactId which manifests itself in the bom, because maven-release-plugin does not see this non existing module and fails to update them.

<artifactId>netty-transport-unix-common</artifactId> should become <artifactId>netty-transport-native-unix-common</artifactId> everywhere. Note the missing native in the name.

tsachev commented Jul 19, 2017

I do not have an opinion on using project.version (unless it makes the bom useless as parent, but that is not its purpose anyway).
There might be something in the release process that replaces ${project.version} with the actual project version I don't know.

But #6979 is about using wrong artifactId which manifests itself in the bom, because maven-release-plugin does not see this non existing module and fails to update them.

<artifactId>netty-transport-unix-common</artifactId> should become <artifactId>netty-transport-native-unix-common</artifactId> everywhere. Note the missing native in the name.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 19, 2017

Member

@tsachev doh! I missed this ... so I think this is the real issue then.. Let me update the pr to just fix the artifactId, this should be good enough.

Member

normanmaurer commented Jul 19, 2017

@tsachev doh! I missed this ... so I think this is the real issue then.. Let me update the pr to just fix the artifactId, this should be good enough.

Correct typo in artifactId of dependency in bom pom.xml
Motivation:

There was a typo in a dependency in the bom pom.xml which lead to have it specify a non-existing artifact and also so not have the maven release plugin update the version correctly.

Modifications:

Rename netty-transport-unix-common to netty-transport-native-unix-common and also fix the version.

Result:

Fixes [#6979]

@normanmaurer normanmaurer changed the title from Use project.version property to specify the version of the dependenci… to Correct typo in artifactId of dependency in bom pom.xml Jul 19, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Jul 19, 2017

@wilkinsona

This comment has been minimized.

Show comment
Hide comment
@wilkinsona

wilkinsona Jul 19, 2017

Contributor

LGTM

Contributor

wilkinsona commented Jul 19, 2017

LGTM

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 19, 2017

Member

Cherry-picked into 4.1 (deb5c45) and committed a updated version to 4.0 (ae66f3a)

Member

normanmaurer commented Jul 19, 2017

Cherry-picked into 4.1 (deb5c45) and committed a updated version to 4.0 (ae66f3a)

@normanmaurer normanmaurer deleted the bom_fix branch Jul 19, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 19, 2017

Member

lgtm

Member

Scottmitch commented Jul 19, 2017

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment