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

Added support for PR builds on master #3363

Merged
merged 5 commits into from Oct 5, 2023

Conversation

Fmstrat
Copy link
Contributor

@Fmstrat Fmstrat commented Oct 3, 2023

  • Add docker JDK builder for PRs into main as a :beta
  • Replace openjdk with sapmachine as the original repo is deprecated, and sapmachine is official now
  • Use ibm-semeru-runtimes (also official) for JDK/JRE 17 LTS

Held up by #3362

Copy link
Owner

@iBotPeaches iBotPeaches left a comment

Choose a reason for hiding this comment

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

Possible to use layer/targets? Want to avoid becoming one of those projects that has like 15 Dockerfile files in root.

docker/build-push-action@v4 supports target.

So the dockerfile could evolve into like:

  • Base
  • Beta
  • Prod (?)

So we only have 1.

@Fmstrat
Copy link
Contributor Author

Fmstrat commented Oct 3, 2023

Yea, I actually started this using INCLUDE+ but since only the absolute latest versions of Docker support it, I stopped that path.

After that I reorganized the Dockerfile but I now notice the install can always happen last, so targets should now work. I'll shift there.

EDIT: I also hate the fill-up of root, so you'll see I moved everything Docker related into ./docker

EDIT 2: Hmm, the problem with target is that we can only stop at one spot. Which means to have one Dockerfile we'd need to do extra steps. I guess we could just build a release vs download one so they're the same.

@Fmstrat
Copy link
Contributor Author

Fmstrat commented Oct 3, 2023

There we go, a single Dockerfile. Releases will be a bit slower because they'll build, but not really a big deal, and we retain the smaller image size. I can test this more fully once #3362 is solved.

@Fmstrat
Copy link
Contributor Author

Fmstrat commented Oct 3, 2023

Also, let me know if you'd prefer the beta workflow to be part of build.yml so that if a previous step fails they're connected.

@iBotPeaches
Copy link
Owner

Also, let me know if you'd prefer the beta workflow to be part of build.yml so that if a previous step fails they're connected.

There is a good deal of duplication, but since the entry point is different (published release vs merged pr) - I imagine that'll be difficult.

I'm guessing what might be easier is a composite workflow action we dump into .github/actions/something and that could do the publishing with inputs given from the respective workflows.

@Fmstrat
Copy link
Contributor Author

Fmstrat commented Oct 3, 2023

Good point, that being said, it only triggers on a completed PR, so all the tests would have already run against the branch, so it probably doesn't actually matter.

@Fmstrat Fmstrat changed the title DRAFT: Added support for PR builds on master Added support for PR builds on master Oct 4, 2023
@Fmstrat
Copy link
Contributor Author

Fmstrat commented Oct 4, 2023

Updated to JDK/JRE 17 LTS. No longer blocked.

@iBotPeaches iBotPeaches merged commit cc5a8ba into iBotPeaches:master Oct 5, 2023
31 checks passed
@iBotPeaches iBotPeaches added this to the v2.9.0 milestone Oct 6, 2023
jabagawee added a commit to jabagawee/Apktool that referenced this pull request Oct 20, 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

2 participants