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

Perform better checks in our start up script #384

Merged
merged 11 commits into from
Jul 18, 2023
Merged

Perform better checks in our start up script #384

merged 11 commits into from
Jul 18, 2023

Conversation

Fweddi
Copy link
Contributor

@Fweddi Fweddi commented Jul 14, 2023

What does this change?

We now check for java and node versions in our start-up scripts. If nvm is installed, we switch node versions automatically.

The build.sbt file also checks for the java version - so we get the same wins if we run the code from within sbt.

There are also a couple of other quick wins around checking for PIDS before we kill them, and tearing down our dependencies in more fail situations.

@Fweddi Fweddi requested a review from a team as a code owner July 14, 2023 16:56
Fweddi
Fweddi commented Jul 14, 2023
View reviewed changes
script/lib/check Outdated Show resolved Hide resolved
@Fweddi
Run checks from the general start script
06e3b31 
If the checks have already been run, we pass this property to the individual starter scripts to avoid them being run three times
@Fweddi
Copy link
Contributor Author

Fweddi commented Jul 17, 2023 •
edited
Loading

I've addressed these comments by removing prescription for any particular manager. If you have fnm, it will try to run that. If you don't, or if fnm fails (as it does for me!), then it will try to use nvm. And if you have neither of those, you have the option of changing manually (using your version manager of choice).

I have also copied the checks into the main start script, passing a --checks-complete flag to the individual scripts. This means the checks are performed only once when running the main start script.

Fweddi
Fweddi commented Jul 17, 2023
View reviewed changes
script/start Show resolved Hide resolved
jonathonherbert
jonathonherbert approved these changes Jul 17, 2023
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.

Works really nicely. I wonder how we'd scale this out to other tools, it'd be lovely if this were the standard everywhere (but difficult to maintain.)

A few non-blocking comments.

build.sbt
@@ -86,7 +100,16 @@ def playProject(projectName: String, devHttpPorts: Map[String, String]) =
s"-J-Dlogs.home=/var/log/${packageName.value}",
s"-J-Xloggc:/var/log/${packageName.value}/gc.log"
),
commonSettings
commonSettings,
initialize := {
Copy link
Contributor

@jonathonherbert jonathonherbert Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason we take this step in sbt, rather than the shell script? Maybe it's a pain to script? (We do every other check there IIUC)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We perform this step in both script and sbt. This might be overkill, but I'm thinking there are a few cases when we run the app / tests / compilation etc. in sbt. In those cases the script checks won't save us.

You could argue this means we should get rid of the script checks, but I feel those are useful to catch the java version error early.

script/lib/check Outdated Show resolved Hide resolved
@Fweddi
Copy link
Contributor Author

Fweddi commented Jul 18, 2023

Works really nicely. I wonder how we'd scale this out to other tools, it'd be lovely if this were the standard everywhere (but difficult to maintain.)

A few non-blocking comments.

I think there's a hope here that we can copy these patterns to other tools. It would be great if we had a more standard way of doing this, which didn't involve dozens of bash scripts copied and pasted across repositories.

@Fweddi Fweddi merged commit 41b49b7 into main Jul 18, 2023
@Fweddi Fweddi deleted the better-checks branch July 18, 2023 09:25
@samanthagottlieb samanthagottlieb mentioned this pull request Jul 20, 2023
Merged
@rtyley
Copy link
Member

rtyley commented Aug 8, 2023

Uh oh! An unfortunate unanticipated consequence of this change is that the sbt initialize setting added with this PR (checking the Java version) causes a job 'failure' for our Scala Steward process - which is running with Java 11:

java.lang.AssertionError: assertion failed: Java 8 is required for this project. You are using Java 11.

image

  • Run 2577 - last successful run 7:55pm on 17th July 2023
  • Run 2588 - next run, failed at 10:55am 18th July 2023

The error doesn't actually kill the Scala Steward process (other repos get updated successfully), but it does cause the entire job to be reported as a 'failure' (which is a bit noisy), and it's still ongoing (ie the last 3 weeks!).

Even worse, because the job 'fails' I think the GitHub Action is not caching, which has caused the execution time to gradually increase from 3 mins to 25 mins, and Scala Steward to forget when it last opened PRs for dependencies, leading to it offering more frequent updates than it should.

Some possible fixes:

  • Update to Java 11 - obviously, this would be good!
  • Make the check in sbt more tolerant - ie accept Java 8 or above

Could you take a look at this @Fweddi ?

@jonathonherbert
Copy link
Contributor

jonathonherbert commented Aug 8, 2023 via email

@jonathonherbert
Copy link
Contributor

jonathonherbert commented Aug 8, 2023 via email

@rtyley rtyley mentioned this pull request Aug 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhystmills rhystmills rhystmills left review comments

@jonathonherbert jonathonherbert jonathonherbert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Fweddi @rtyley @jonathonherbert @rhystmills