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

Upgrade to spring-boot 3 / java 17 #7416

Closed
wants to merge 143 commits into from
Closed

Conversation

bogdan-sava
Copy link
Contributor

Upgrade dependencies to use spring-boot 3.
Change the code to use spring-boot 3.

@planetf1
Copy link
Member

@bogdan-sava Thanks for proposing the Spring 3 update, which isn;t trivial! I'd like to review when appropriate. I'll plan to wait until the PR comes out of draft as you work through it.


/**
*
* @return CorsConfigurationSource the cors configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

38% of developers fix this issue

MissingSummary: A summary fragment is required; consider using the value of the @return block as a summary fragment instead.


Suggested change
* @return CorsConfigurationSource the cors configuration
*Returns corsConfigurationSource the cors configuration

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.


Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@sonatype-lift
Copy link
Contributor

sonatype-lift bot commented Feb 22, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/odpi/egeria/7416.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/odpi/egeria/7416.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

@bogdan-sava bogdan-sava force-pushed the version4 branch 2 times, most recently from 051c907 to ce05742 Compare February 23, 2023 09:24
Copy link
Member

@planetf1 planetf1 left a comment

Choose a reason for hiding this comment

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

Looks great. only a minor comment on dependency scope. It's pointed out some things I missed outside the spring scope also which I can look at in a future PR.

Looking at codeql issue next

build.gradle Outdated
exclude("junit:junit", "org.junit.jupiter:junit-jupiter-api",
"org.junit.jupiter:junit-jupiter-engine", "com.fasterxml.jackson.core:jackson-annotations",
"org.springframework.boot:spring-boot-starter-oauth2-resource-server",
"org.springdoc:springdoc-openapi-starter-webmvc-ui",
Copy link
Member

Choose a reason for hiding this comment

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

I think the spring exclusions can be removed. with just one change later they are not needed - so dependencies are correct. That one change relates to making the springdoc dependency 'runtimeOnly'.

This is a reminder there are some other cleanups that should be done with gradle/ junit, but I will look at that once your spring change is merged

I am also ok if you want to merge as-is - I don't notice anything that would cause problems/regression

I will look at the junit/jupiter/jackson in a separate issue after merging - that isn't specific to your spring change

@planetf1
Copy link
Member

Looking at the codeQL issue - github/codeql#8712 is related. It seems as if the sarif file (which contains info about components and scan results) is > 512MB. The referenced issue talks though how to identify the cause. Most likely it's due to a lot of scanning findings added for springboot3/spring6

Either we can try and debug this now -- there may be genuine issues we want to fix, or, pragmatically we set codeQL to no longer be an enforced check & merge anyway. Since we are in the middle of a longer release cycle I think that would be ok. The risk is that there is a genuine issue found, but equally it could be very minor, or a scanner bug!

Interestingly sonatype was fairly happy....

popa-raluca and others added 19 commits February 28, 2023 08:54
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
Signed-off-by: Iliescu Cristian-Mihai <cristianmihaiiliescu@gmail.com>
bogdan-sava and others added 12 commits February 28, 2023 09:02
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
…ssCall

Signed-off-by: Raluca Popa <raluca.popa2@ing.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
@bogdan-sava bogdan-sava force-pushed the version4 branch 2 times, most recently from 6d310cb to 040dc93 Compare February 28, 2023 07:07
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
Signed-off-by: Bogdan Sava <bogdan.sava@gmail.com>
@bogdan-sava
Copy link
Contributor Author

I did a change in the codeQL workflow, it is fail but at least now shows the report.
The problem is that it shows a lot of issues that aren't related to the PR.

@planetf1
Copy link
Member

planetf1 commented Mar 1, 2023

The commit history here has gone somewhat awry as there are a lot of commits already in master.

I recommend the PR is rebuilt based off current master via cherry-pick or rebase, as otherwise
a) the list of change is confusing to review
b) there's a risk of regression if any changes are accidentally undone

@planetf1
Copy link
Member

planetf1 commented Mar 1, 2023

the changes looked ok until ced86f3

Another option may be to revert back to that version, and then pull from master.
Or potentially rebase (this worked well when I was working on changes for v4)

@planetf1
Copy link
Member

planetf1 commented Mar 1, 2023

  • I notice codeQL is no longer failing - it's just reporting alerts. I suggest leaving the change you made to the codeQL build in place, in case it fails again -- wan then followup with the dev team
  • it's hard to see where all the error reports are - or why we suddenly got more, but I think it may be best to skip over the check for now. We know we have >2600 observations to action and need a seperate work thread to figure out how to deal with them
  • As above, this PR seems to have an odd/divergent history and should be rebuilt as it could cause odd merge behaviour, I think it's important we are clean. This might mean either cherry-picking, unwinding to before the last merge, rebasing (happy to take a look if it helps)

@planetf1 planetf1 closed this Mar 2, 2023
@planetf1 planetf1 reopened this Mar 2, 2023
@planetf1
Copy link
Member

planetf1 commented Mar 3, 2023

@bogdan-sava let me know if you need help with any untangling/cherry-pick/rebase -- hopefully the fixes themselves are good, and there's just some git sorting to do.

Copy link
Member

@planetf1 planetf1 left a comment

Choose a reason for hiding this comment

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

Set review status - PR needs a rebuild.

@planetf1
Copy link
Member

planetf1 commented Mar 9, 2023

@lpalashevski @mandy-chessell @bogdan-sava I offer #7506 as a cleaned-up PR which addresses the commit history/merge observations. It has 15 commits vs 143 here

See PR for more info, but I don't profess to offer anything new, I've just cherry-picked from the original work (which retains commit credit).

@planetf1
Copy link
Member

replaced by #7506

@planetf1 planetf1 closed this Mar 13, 2023
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.

None yet

6 participants