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

Improved build process #1566

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Improved build process #1566

merged 7 commits into from
Apr 29, 2024

Conversation

fredex42
Copy link
Contributor

@fredex42 fredex42 commented Apr 25, 2024 •
edited
Loading

What's changed?

  • Removes the old ci.sh build script
  • Replaces it with paralleling Github actions
  • Removes the deprecated SBT riffraff plugin in favour of most recent GHA version
  • Build on Java11, compile Scala optimised for java 11
  • Update the target AMI to Java11 as well

Implementation notes

None

Checklist

Things that still need checking:

  • There are errors being reported of files not found during the frontend v2 integration tests. These do not fail the tests and I have a suspicion that they relate to the Webpack->Vite migration but not sure. [FIXED in https://github.com/Remove red ink in integration test runs #1568]
  • I'm not familiar enough with the frontend to be able to check everything, but what I do know about looks OK

General

  • 🤖 Relevant tests added <-- n/a
  • ✅ CI checks / tests run locally
  • 🔍 Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • 🎛️ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work) <-- partially checked
  • 📷 Screenshots / GIFs of relevant UI changes included [None]

@fredex42 fredex42 requested a review from a team as a code owner April 25, 2024 14:16
@fredex42 fredex42 marked this pull request as draft April 25, 2024 14:16
@fredex42 fredex42 force-pushed the ag/improved-build-process branch 5 times, most recently from 64464ea to 1449005 Compare April 25, 2024 15:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 25, 2024 •
edited
Loading

Deploy build 7987 of cms-fronts::facia-tool to CODE

All deployment options

From guardian/actions-riff-raff.

@fredex42 fredex42 force-pushed the ag/improved-build-process branch 3 times, most recently from 35f4f81 to a47e49b Compare April 25, 2024 16:26
@fredex42 fredex42 marked this pull request as ready for review April 25, 2024 16:27
@fredex42 fredex42 mentioned this pull request Apr 26, 2024
Merged
6 tasks
@fredex42 fredex42 force-pushed the ag/improved-build-process branch from a47e49b to 7278bcd Compare April 26, 2024 08:09
@jonathonherbert
Copy link
Contributor

This looks great! A big improvement.

A bit of work to do on the v1 client, as currently it's throwing an error:
Screenshot 2024-04-26 at 09 52 20
Unsure off top of head of a fix, but v. happy to pair.

@jonathonherbert jonathonherbert mentioned this pull request Apr 26, 2024
Merged
6 tasks
@fredex42
Copy link
Contributor Author

@jonathonherbert thanks for that. Looks like it's not bundling the JSPM deps...

@fredex42
Copy link
Contributor Author

@jonathonherbert thanks for that. Looks like it's not bundling the JSPM deps...

Fixed this morning :)

jonathonherbert
jonathonherbert approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

Tested:

  • V2 client (fronts edits, proof an edition)
  • V1 client (config, treats)
  • Breaking news (sent notification)

Bloomin' lovely. A few non-blocking comments.

build.sbt
import sbt.Resolver

debianPackageDependencies := Seq("openjdk-8-jre-headless")
debianPackageDependencies := Seq("java11-runtime-headless")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FMI as I'm sure it is – why is this necessary? Doesn't the AMI provide a Java runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it's not necessary - but it's good practise when building a java-based DEB to have a generic JRE dependency

Copy link
Member

Choose a reason for hiding this comment

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

Andy is correct that technically it isn't necessary. But if this line is present, it needs to be correct. AFAIK if you left this as openjdk-8-jre-headless the package would fail to install.

.github/workflows/improvedci.yml Outdated Show resolved Hide resolved
@fredex42 fredex42 merged commit c4882d9 into main Apr 29, 2024
9 checks passed
@fredex42 fredex42 deleted the ag/improved-build-process branch April 29, 2024 11:16
@prout-bot
Copy link

Seen on PROD (merged by @fredex42 20 minutes and 2 seconds ago) Please check your changes!

@jonathonherbert jonathonherbert mentioned this pull request May 3, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfurey davidfurey davidfurey left review comments

@jonathonherbert jonathonherbert jonathonherbert approved these changes

Assignees
No one assigned
Labels
Seen-on-PROD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fredex42 @jonathonherbert @prout-bot @davidfurey