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 Spring Boot & Gradle Wrapper versions across the board #615

Merged
merged 11 commits into from
Oct 2, 2023

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Aug 25, 2023

Summary

Update everything in one PR.

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@dmikusa dmikusa added dependencies Pull requests that update a dependency file java Pull requests that update Java code labels Aug 25, 2023
@anthonydahanne
Copy link
Member

is it stuck because of Spring Boot gradle upgrades?

@dmikusa
Copy link
Contributor Author

dmikusa commented Aug 31, 2023

is it stuck because of Spring Boot gradle upgrades?

No, this one is stuck because of caching problem. We cache all of the gradle/maven downloads on the host runner and then volume map those into the build containers so that each application doesn't need to re-download the world.

This requires getting the permissions correct so that the build containers can write to these locations. At the moment, that is failing and it can't write. It was working, so something has changed but we're not sure what yet. When this happened previously, something had changed in the Github Ubuntu "latest" image.

Basically, someone has to go through and check permissions/ownership outside of the container and inside of the container and then compare the two. Hopefully, we can spot what changed and adjust the workflow.

Copy link
Contributor Author

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

Thoughts

.github/workflows/test-pull-request-java-akka.yml Outdated Show resolved Hide resolved
.github/workflows/test-pull-request-java-akka.yml Outdated Show resolved Hide resolved
java/akka/smoke_test/akka_test.go Outdated Show resolved Hide resolved
java/akka/smoke_test/akka_test.go Show resolved Hide resolved
@anthonydahanne
Copy link
Member

Hello @dmikusa
So this is what I want to generalize for all Java samples (for now just 2)
https://github.com/paketo-buildpacks/samples/actions/runs/6294008765
Continuing refactoring the other samples

@dmikusa
Copy link
Contributor Author

dmikusa commented Sep 25, 2023

Hello @dmikusa So this is what I want to generalize for all Java samples (for now just 2) https://github.com/paketo-buildpacks/samples/actions/runs/6294008765 Continuing refactoring the other samples

That's interesting, so instead of using the trigger to execute the workflow based on the path, you have one workflow that runs and then determines what to execute and kicks off other things via a matrix. Did I follow that right?

I don't have strong feelings about the approach so long as we are only testing things that have changed. I do generally have an aversion to lots of inline bash. We have that in a few places and it's a nightmare to maintain. You might want to consider replacing that with an action (i.e. some Go code and unit tests).

If your concern with generalizing stuff is reducing duplication in the workflows, you might also consider a reusable workflow.

@anthonydahanne
Copy link
Member

That's interesting, so instead of using the trigger to execute the workflow based on the path, you have one workflow that runs and then determines what to execute and kicks off other things via a matrix. Did I follow that right?

That is correct: there's the first job, named prepare that uses git history to find out what are the Java samples that got changed, then it creates a list (as seen in the run summary) of all the Java samples paths that do have a smoke_test folder and have changed. Then, a job running smoke_test will be created for each element of the list.

I don't have strong feelings about the approach so long as we are only testing things that have changed.

Right, this is exactly what it does

I do generally have an aversion to lots of inline bash. We have that in a few places and it's a nightmare to maintain. You might want to consider replacing that with an action (i.e. some Go code and unit tests).

I understand your concern. I usually also dislike bash scripts for anything bigger than 3 lines of code; but...

  • I started with bash re using some logic I already successfully (as in no maintenance) implemented on another project
  • the situation is critical for the samples nothing runs for Java; I don't like to use the emergency argument, but rewriting in Go with an image published, etc. well, it will push the fix further away I believe...

If your concern with generalizing stuff is reducing duplication in the workflows, you might also consider a reusable workflow.

oh, interesting; but my concern was not only about copy/pasting (which re usable workflows in a way solve); but also in the fact of creating a test-pull-request-XXX.yml for every and each project; which reusable workflows won't solve unfortunately

@dmikusa
Copy link
Contributor Author

dmikusa commented Sep 25, 2023

I understand your concern. I usually also dislike bash scripts for anything bigger than 3 lines of code; but...

Yes, I get the time crunch here. My counter-proposal would be make sure it's using set -eo pipefail at the top and very heavy comments in the bash script. Explain what everything is doing.

@anthonydahanne
Copy link
Member

anthonydahanne commented Sep 27, 2023

pretty much all set now @dmikusa .

I'll rebase it on #625 once it's merged (it will fix the only failing smoke test) and then we're good I believe!

@dmikusa
Copy link
Contributor Author

dmikusa commented Sep 27, 2023

It's looking good. Only question. I see that the caching host-level caching for Gradle/Maven was removed. Even splitting things up, that's going to help speed up runs. The download time for dependencies is about 1/3 of the build time for Java apps (non-native). Still seems significant. Wondering your thoughts on removing it.

@anthonydahanne
Copy link
Member

Hello 👋

I see that the caching host-level caching for Gradle/Maven was removed. Even splitting things up, that's going to help speed up runs.

What caching precisely are you referring to?

In any way, I believe caching for the samples to not be helping.
Sure, we could save some seconds here and there; but this is not what our users are going to experience when they use the paketo buildpacks on their machines: they'll go through long downloads and potential missing artifacts. As we've ourselves experienced recently some discrepancies between the cached samples and samples builds from scratch, caching can in our case hide environmental issues that we need to notice before our users do.

@anthonydahanne anthonydahanne mentioned this pull request Sep 27, 2023
@anthonydahanne anthonydahanne enabled auto-merge (squash) September 27, 2023 21:27
@anthonydahanne anthonydahanne merged commit a7f57b8 into main Oct 2, 2023
24 checks passed
@anthonydahanne anthonydahanne deleted the sb-update branch October 2, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file java Pull requests that update Java code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants