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

[MSHADE-425] Relocate services name before add to serviceEntries #150

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Sep 3, 2022

Signed-off-by: tison wander4096@gmail.com

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MSHADE-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MSHADE-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 3, 2022

cc @khmarbaise @rmannibucau

@slawekjaranowski
Copy link
Member

@tisonkun Thanks for your PR.
We need a unit test to be sure that next change don't break it.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

@slawekjaranowski Yes. I ever thought of this. Let me try to dig out how to add one. Or could you show me some related tests.

@slawekjaranowski
Copy link
Member

I don't investigate deeper - only mention about missing test

@slawekjaranowski
Copy link
Member

There is a https://github.com/apache/maven-shade-plugin/blob/master/src/test/java/org/apache/maven/plugins/shade/resource/ServiceResourceTransformerTest.java

maybe tests don't cover all path - you change something in class but tests still works without change

@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

@slawekjaranowski Yes. I would say this patch is simple enough to be correct. But I'm glad to add some tests to prevent regressions.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

@slawekjaranowski Added at d29c633. Also verify locally that without this patch the test failed.

@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

@rmannibucau @slawekjaranowski do we still wait for other conditions to merge this patch?

@rmannibucau
Copy link
Contributor

@tisonkun on my side I was just waiting for @slawekjaranowski to ack (or merge, no issue) but otherwise no blocker for this pr for me

@slawekjaranowski
Copy link
Member

you can squash to one final commit and will be ok for me also.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun force-pushed the relocate-services-name branch from 6d59e50 to a6848a0 Compare September 5, 2022 09:19
@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

Squashed.

Though, you can use GitHub "Squash and Commit" button to perform the squash, so that we don't need one more CI turn.

@slawekjaranowski slawekjaranowski merged commit ad2f6f8 into apache:master Sep 5, 2022
@tisonkun tisonkun deleted the relocate-services-name branch September 5, 2022 12:45
@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

@rmannibucau @slawekjaranowski I may be a bit in a hurry but I'd like to know whether/when we can have a release for this fix. It resolves one or several downstream use cases and I'm happy to upgrade for this fix.

It's not a request, though.

@rmannibucau
Copy link
Contributor

@tisonkun guess a mail on the mailing list can make people more aware of it (at least the ones with the full karma to do it ;))

@slawekjaranowski
Copy link
Member

drop a line on dev list is good idea
@rmannibucau each committer has enough karma/privileges for releases

@rmannibucau
Copy link
Contributor

@slawekjaranowski nop, only a PMC can achieve it (for goods) so if a commiter can start it he can't end it so always better to ensure some PMC is there to handle all the needed steps before starting by XP.

@slawekjaranowski
Copy link
Member

if you wish we can discuss on dev list
look last pending release of m-pmd-p

@tisonkun
Copy link
Member Author

tisonkun commented Sep 5, 2022

Thanks! I'm subscribing dev@maven.apache.org mailing list now.

m-pmd-p

Out of curiosity, what does this stand for?

@slawekjaranowski
Copy link
Member

m-pmd-p -> maven-pmd-plugin 😄

tisonkun added a commit to tisonkun/pulsar that referenced this pull request Sep 15, 2022
This closes apache#17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>
merlimat pushed a commit to apache/pulsar that referenced this pull request Sep 15, 2022
This closes #17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>
Jason918 pushed a commit to apache/pulsar that referenced this pull request Sep 26, 2022
This closes #17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>
(cherry picked from commit 2865007)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 28, 2022
This closes apache#17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>
(cherry picked from commit 2865007)
(cherry picked from commit f53c9d5)
congbobo184 added a commit to apache/pulsar that referenced this pull request Nov 8, 2022
This closes #17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.
congbobo184 pushed a commit to apache/pulsar that referenced this pull request Nov 30, 2022
This closes #17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>
(cherry picked from commit 2865007)
nodece pushed a commit to apache/pulsar that referenced this pull request Jun 7, 2023
This closes #17047.

Upstream patch is apache/maven-shade-plugin#150.

Bump maven-shade-plugin version to 3.4.0 for the upstream fix.

Signed-off-by: tison <wander4096@gmail.com>

Signed-off-by: tison <wander4096@gmail.com>
(cherry picked from commit 2865007)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants