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

Igarish snow 804145 streaming sdk gs rules #1382

Merged
merged 20 commits into from
May 30, 2023

Conversation

sfc-gh-igarish
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish commented May 5, 2023

Overview

SNOW-804145
This PR inherits the work from #1318. Basically adding changes to make it green in JDBC repo.

changes in original PR

#1318 (comment)

new changes on top of the original PR

#1382 (comment)

Note for the abnormal review / approval process.

The PR was originally created by @sfc-gh-igarish and then taken over by @sfc-gh-wshangguan . So @sfc-gh-igarish should be the reviewer in JDBC team. But since the PR creator can't approve the PR, @sfc-gh-wshangguan will give approval though he is the one who requested reviews. Next time should consider opening a new PR to avoid confusions.

External contributors - please answer these questions before submitting a pull request. Thanks!

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes #NNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

Pre-review checklist

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

@sfc-gh-igarish sfc-gh-igarish requested a review from a team as a code owner May 5, 2023 19:03
Preston4tw and others added 19 commits May 26, 2023 23:44
- fix pom.xml url
- add sortpom plugin and sort pom.xml
- remove internal repository reference
- fix scm section
- add a pluginManagement section
- add version properties for plugins
- update plugin versions
- add dependency convergence and upper bounds checks
- fill out dependencyManagement section to converge all dependencies at the upper bound
- add unused declared and used undeclared dependency checks
- remove unused declared dependencies
- add used undeclared dependencies
- fix the <scope> declaration of several dependencies
- remove all dependency <exclusions> sections
- remove maven-install-plugin
- add japicmp plugin to enforce semantic versioning
- update maven required version to 3.6.3
- add linkage checker enforcer rule
- add linkage exclusions covering the current set of linkage issues
- add duplicate class enforcer check from extra-enforcer-rules
- adjust the java-9 profile to activate for jdks 9 or greater
- add JPMS line for tests with jdk>8 "--add-opens=java.base/java.nio=ALL-UNNAMED"
- update fmt-maven-plugin
- add maven wrapper, pin maven to 3.8 for compatibility with linkage checker
- disable shading by default, fixes #174 #608
- dependency fixes possibly fix #1211
- update json-smart to 2.4.9, related #1311
- httpclient to 4.5.14, #1273
… because java 8 in test infra doesn't support release flag.
@sfc-gh-wshangguan sfc-gh-wshangguan force-pushed the igarish-SNOW-804145-streaming-sdk-gs-rules branch from 53339f8 to 53f306d Compare May 26, 2023 23:52
@sfc-gh-wshangguan sfc-gh-wshangguan force-pushed the igarish-SNOW-804145-streaming-sdk-gs-rules branch from a3c8a19 to 1c7f723 Compare May 27, 2023 00:55
@sfc-gh-wshangguan
Copy link
Contributor

Changes since the original PR.

  1. downgrade sortpom's version to 3.0.1 for java8; (3.2.1 is compiled by java11)
  2. use src, target flag instead of release flag in maven-compiler-plugin as java8 doesn't support it
  3. disable enforce-linkage-checker in shaded-jar mode
  4. change slf4j to scope - provided
  5. disable japicmp when tests run on jenkins
  6. disable maven-dependency-plugin:analyze-only when tests run on jenkins
  7. change default build back to fat-jar

cc @sfc-gh-pbennes please review whether the above changes look good.

@sfc-gh-wshangguan
Copy link
Contributor

@sfc-gh-igarish Please also review the change. I can't add you as reviewer since you created this PR.

Copy link

@sfc-gh-pbennes sfc-gh-pbennes left a comment

Choose a reason for hiding this comment

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

LGTM. We'll need to figure out how to get the linkage checker enabled and non-fat-jars published in future PRs.

@sonarqubemergegate
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sfc-gh-wshangguan
Copy link
Contributor

@sfc-gh-pbennes Thank you for the review, so for the linkage checker, it's enabled in non-fat-jar, which is used at least in jenkins test (not sure what's the case in github action). Does this sound good enough?

About the non-fat-jar publish, sure, let's reserve a time to discuss it offline.

@sfc-gh-pbennes
Copy link

Yeah that's a good first step. It might be tedious to add exclusions for the not-fat-jar for the initial internal inclusion but it should get better once the not-fat-jar is available to consume.

@sfc-gh-igarish
Copy link
Collaborator Author

LGTM

Copy link
Contributor

@sfc-gh-wshangguan sfc-gh-wshangguan left a comment

Choose a reason for hiding this comment

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

Verified that mvn clean install -Dnot-self-contained-jar generates non-shaded jar, while the default mvn clean install creates shaded-jar, I.e. no change to the default build behavior.

Approve on behalf of @sfc-gh-igarish .

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