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.

Sign up for GitHub

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

Jump to bottom

Build polish and upgrade, switch to Gradle 6.8.3, snapshot of 1.0.5 #175

Merged
merged 4 commits into from
Mar 23, 2021
Merged

Build polish and upgrade, switch to Gradle 6.8.3, snapshot of 1.0.5 #175

merged 4 commits into from
Mar 23, 2021

Conversation

simonbasle
Copy link
Contributor

@simonbasle simonbasle added type/chore A task not related to code (build, formatting, process, ...) type/dependency-upgrade A dependency upgrade labels Mar 23, 2021
@simonbasle simonbasle self-assigned this Mar 23, 2021
@simonbasle simonbasle changed the title Build polish and upgrade, switch to Gradle 6.8.3 Build polish and upgrade, switch to Gradle 6.8.3, snapshot of 1.0.5 Mar 23, 2021
simonbasle
simonbasle commented Mar 23, 2021
View reviewed changes
agent/build.gradle Outdated Show resolved Hide resolved
@simonbasle simonbasle requested a review from a team March 23, 2021 14:02
OlegDokuka
OlegDokuka approved these changes Mar 23, 2021
View reviewed changes
Copy link

@OlegDokuka OlegDokuka left a comment

Choose a reason for hiding this comment

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

LGTM but I would defer merging till @bsideup comments

@simonbasle
Copy link
Contributor Author

NB: I have validated the current commit works by using jitpack in a demo project that installs the agent programmatically and flags a Thread.sleep.

@simonbasle
Build polish: bump Gradle to 6.8.3 and fix build issues (#175)
bdfaa8b 
 - annotation processing now must be defined using `annotationProcessor`
 dependency type
 - use archiveClassifier.set syntax
 - use implementation/runtime syntax for dependencies
@simonbasle simonbasle force-pushed the buildPolishMarch2021 branch from c30671a to bdfaa8b Compare March 23, 2021 18:23
@simonbasle
Copy link
Contributor Author

I'll merge this for now, but don't hesitate to chime in @bsideup, your input will be appreciated 👍

@simonbasle simonbasle merged commit 727263a into master Mar 23, 2021
simonbasle added a commit that referenced this pull request Mar 23, 2021
@simonbasle
Fix current snapshot 1.0.5 not 1.0.4 (#175)
13870fc
simonbasle added a commit that referenced this pull request Mar 23, 2021
@simonbasle
Build polish: remove jcenter() in favor of mavenCentral() (#175)
5ceee35
simonbasle added a commit that referenced this pull request Mar 23, 2021
@simonbasle
Build polish: add simonbasle as dev (#175)
c25a96c
@simonbasle simonbasle deleted the buildPolishMarch2021 branch March 23, 2021 18:30
@bsideup
Copy link
Contributor

bsideup commented Mar 24, 2021

@simonbasle I am curious what was the motivation to merge this so fast, outside of work hours (19:30)?

Also, the branch name (buildPolishMarch2021) kinda suggests that the "normal" approach (1 PR per change) was ignored. Have you changed it and started doing monthly massive PRs? 😅

@bsideup
Copy link
Contributor

bsideup commented Mar 24, 2021

JFYI I am no longer receiving the mention notifications, as I was force removed from the org, and I cannot change my notifications anymore (I had reactor org notifications configured to go to my Pivotal email)

@simonbasle
Copy link
Contributor Author

@bsideup yeah I wanted to move quickly on this to get JDK 16 compatible snapshots (and fix the bad snapshot version). I could have done multiple PRs I guess, but I preferred these build-related changes to be reviewed together, as it didn't feel that massive.

re the notifications, have you tried removing your pivotal email from the github account altogether? that sounds like a github bug :(

@bsideup
Copy link
Contributor

bsideup commented Mar 24, 2021

@simonbasle I cannot remove the email, I'd lose my contributions :D

AFAIK JDK 16 removed the flag that we were relying upon (AllowRedefinitionToAddDeleteMethods).
"JDK 16 compatible snapshots" may not be as easy as updating Gradle, and if I were you, I would start with adding JDK 16 to the test matrix ;)

@simonbasle
Copy link
Contributor Author

It boils down to: I don't think there was actual harm in bumping these dependencies and Gradle, but I could have waited a tiny bit longer before merging. My bad.

AFAIK JDK 16 removed the flag that we were relying upon (AllowRedefinitionToAddDeleteMethods).
"JDK 16 compatible snapshots" may not be as easy as updating Gradle, and if I were you, I would start with adding JDK 16 to the test matrix ;)

Fair point. Let's not advertise JDK16 compatibility just yet, then 😞

Ping me on email if you need help from VMware side to resolve the notification issue.

@bsideup
Copy link
Contributor

bsideup commented Mar 24, 2021

@simonbasle it's okay, these notifications are only for reactor org, all my other notifications go to my personal email.

Once BlockHound goes to the Reactive Foundation, it won't be a problem anymore.

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

@bsideup bsideup bsideup left review comments

@OlegDokuka OlegDokuka OlegDokuka approved these changes

Assignees

@simonbasle simonbasle

Labels
type/chore A task not related to code (build, formatting, process, ...) type/dependency-upgrade A dependency upgrade
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonbasle @bsideup @OlegDokuka