Skip to content

Conversation

@wuchunfu
Copy link
Member

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2021

Codecov Report

Merging #720 (6c8555f) into master (3e8a621) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head 6c8555f differs from pull request most recent head a99772b. Consider uploading reports for the commit a99772b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
- Coverage   80.53%   80.47%   -0.07%     
==========================================
  Files         122      122              
  Lines        4743     4738       -5     
  Branches      574      574              
==========================================
- Hits         3820     3813       -7     
- Misses        601      603       +2     
  Partials      322      322              
Impacted Files Coverage Δ
.../scala/org/apache/kyuubi/server/KyuubiServer.scala 47.36% <0.00%> (-2.64%) ⬇️
...ala/org/apache/kyuubi/session/SessionManager.scala 52.94% <0.00%> (-2.30%) ⬇️
...e/authentication/KyuubiAuthenticationFactory.scala 77.50% <0.00%> (-1.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e8a621...a99772b. Read the comment docs.

@yaooqinn yaooqinn requested a review from pan3793 June 26, 2021 10:48
<maven.plugin.antrun.version>3.0.0</maven.plugin.antrun.version>
<maven.plugin.build.helper.version>3.2.0</maven.plugin.build.helper.version>
<maven.plugin.compiler.version>3.8.1</maven.plugin.compiler.version>
<!-- DO NOT bump 4.4.0, see https://github.com/NetEase/kyuubi/pull/441 -->
Copy link
Member

@pan3793 pan3793 Jun 26, 2021

Choose a reason for hiding this comment

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

Have a verification that #441 still exists in 4.5.3, I think we need to figure out what happens in scala maven plugin before upgrading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I will upgrade the version of scala-maven-plugin to 4.5.3 and pass the local verification, and run the build/dist --tgz command to package normally

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it also passed ci but not the case reported by #441

Copy link
Member Author

Choose a reason for hiding this comment

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

What should I do?

Copy link
Member

@pan3793 pan3793 Jun 26, 2021

Choose a reason for hiding this comment

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

We need to figure out what happens in scala maven plugin cause #441 first.
If it's an accident change, fix it in scala maven plugin;
if scala maven plugin team confirm the change is by design and they don't think #441 should be supported, we can do upgrade and ignore #441.

Copy link
Member

Choose a reason for hiding this comment

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

Let's add a new comment here and skip this version too.

<!-- DO NOT bump 4.4.0, see https://github.com/NetEase/kyuubi/pull/441 -->
<!-- DO NOT bump 4.5.3, see https://github.com/NetEase/kyuubi/issues/708 -->

pom.xml Outdated
<!-- DO NOT bump 4.4.0, see https://github.com/NetEase/kyuubi/pull/441 -->
<maven.plugin.scala.version>4.3.0</maven.plugin.scala.version>
<!-- DO NOT bump 4.5.3, see https://github.com/NetEase/kyuubi/issues/708 -->
<maven.plugin.scala.version>4.5.3</maven.plugin.scala.version>
Copy link
Member

Choose a reason for hiding this comment

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

revert it to 4.3.0

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@yaooqinn I has now been revert and add a new comment here and skip this version too.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

LGTM

@pan3793 pan3793 added this to the v1.3.0 milestone Jun 28, 2021
@pan3793 pan3793 closed this in e8abfad Jun 28, 2021
@pan3793
Copy link
Member

pan3793 commented Jun 28, 2021

Thanks, merged for v1.3.0.

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.

5 participants