Add docker integration, CI, and internal `elm-review` #27

jiegillet · 2022-03-01T14:26:33Z

This PR is meant to address everything left to do before deployment.

Docker

It took me a while to figure out how docker works and how to debug it (especially since both elm and elm-review don't have explicit offline modes), but thanks to useful advice from Slack, I think I managed to get to something that works. I ended up with something very similar to elm-test-runner.
Of course, please do review carefully since this is my first docker build.

I made various changes to the shell scripts so that the same ones can run both locally and in docker.

CI

I copied docker.yml pretty much as is (I only removed the master branch) from an Elixir repo, that should be fine. It doesn't run on my fork, probably @ErikSchierboom has to do some admin magic on it?

elm_tests.yml runs all the tests with elm-test (not elm-test-rs because version 1.1 doesn't allow tests with the same name, but IMO they make sense here. I didn't check later versions), checks the formatting, runs elm-review on the repo, and runs run-tests-in-docker.sh, which is basically bin/smoke_test.sh in the container.

`elm-review`

I took the approach of (quote from README)

As a rule of thumb, if we expect the students to follow specific rules when writing their solutions, we should hold ourselves to the same standards in our source code.

So I would like to keep the rules from the repo the same as the common rules we use for the students (we don't have any active at the moment, but I'm expecting we will add NoUnused and Simplify at some point).

All changes from the elm code was suggested from those rules.

README

I wrote one.

Going live

We will need to merge that PR so we can check if the analyzer works as intended. You will get a comment if you use either List.filter or List.filterMap.

Maintaining the repo

At this point, I would like to request to become a maintainer on the Elm track, if I can get the blessing of @mpizenberg and @ceddlyburge.

I changed the CODEOWNER file so that we don't need admins to review each PR that don't touch the .github repo.

Fix solution_cache.tar path

ErikSchierboom

Great stuff! I've left some comments.

@iHiD Could you give this repo access to the proper secrets?

.github/CODEOWNERS

.github/workflows/elm_tests.yml

Dockerfile

ErikSchierboom · 2022-03-01T14:50:29Z

bin/run-in-docker.sh

+mkdir -p "$OUTPUT_DIR"
+docker run \
+    --rm \
+    --network none \


Just a FYI: we currently don't yet run on a readonly filesystem, but this might change in the future. You don't have to change anything, but just wanted to point it out.

What does readonly mean exactly? Not being able to create any sort of file anywhere on the container?
Because if that's the case, it would become a lot more tricky to make the analyzer or even test runner work...

Complementary remark, the test runner cannot work "simply" in readonly because the elm compiler is writing to a lock file inside $HOME/.elm during compilation. It can be done by changing the location of the $ELM_HOME variable to a writable place during execution but that's very cumbersome for the docker code, because it also means copying the contents of that directory to have all pre-installed packages available.

No network is fine though, my remark was just regarding readonly

What does readonly mean exactly? Not being able to create any sort of file anywhere on the container?
Because if that's the case, it would become a lot more tricky to make the analyzer or even test runner work...

It would mean that by default all directories are indeed not writable, save for a couple of exceptions: the /tmp dir and the /output dir. But as we've not yet implemented this (or decided we want this), I wouldn't worry about it. Note that most of the test runners have been implemented this way (to work with readonly), but for some tracks it can indeed be more of a bother.

If /tmp stays writable, then I believe we can make it work. No worries then :)

ErikSchierboom · 2022-03-01T14:52:55Z

test_data/strain/imperfect_solution/expected_analysis.json

@@ -0,0 +1,2 @@
+{"summary":"Check the comments for things to fix. 🛠 ","comments":[{"comment":"elm.strain.do_not_use_filter","type":"essential","params":{}}]}


It's important to have https://github.com/exercism/website-copy/pull/2161/files merged before the analyzer property is set to true in the track's config.json file: https://github.com/exercism/elm/blob/main/config.json#L19

Noted! I didn't know about that line inconfig.json, thank you.

I don't think I can request a review from @mpizenberg or @ceddlyburge on website-copy, they are not on the list. How can we add them in there? We will need to approve analyzer comments in the future. Maybe in the mean time you could approve it?

I've approved.

I don't think I can request a review from @mpizenberg or @ceddlyburge on website-copy, they are not on the list. How can we add them in there? We will need to approve analyzer comments in the future. Maybe in the mean time you could approve it?

I don't know if @mpizenberg and/or @ceddlyburge want to be added to the website-copy team, as they'll get a lot more notifications then.

We have a separate team for reviewing website-copy PRs, so you could always ping them (@exercism / website-copy).
You could then first request a review from @mpizenberg or @ceddlyburge and when they're happy, ask for the website-copy team to approve and merge.

Indeed, I'd prefer not to be added to website-copy. I don't mind occasionally being pingged there for analyzer messages though, even if I cannot officially review. I might just comment if I find the time, or not if I don't.

I've just merged the website-copy PR

I think I am added to website-copy, but I unwatched the repo, so I only get emails when people mention me or request a review. That works for me.
But sure, we can ping people for analyzer messages.

test_data/strain/perfect_solution/expected_analysis.json

This reverts commit c4464fd.

ceddlyburge · 2022-03-01T16:22:57Z

At this point, I would like to request to become a maintainer on the Elm track, if I can get the blessing of @mpizenberg and @ceddlyburge.

You have my blessing :)

mpizenberg · 2022-03-01T16:26:10Z

Mine too :)

jiegillet

Thank you very much.
I think I've addressed all the comments.

Dockerfile

jiegillet · 2022-03-02T01:27:04Z

bin/run-in-docker.sh

+mkdir -p "$OUTPUT_DIR"
+docker run \
+    --rm \
+    --network none \


What does readonly mean exactly? Not being able to create any sort of file anywhere on the container?
Because if that's the case, it would become a lot more tricky to make the analyzer or even test runner work...

jiegillet · 2022-03-02T01:33:20Z

test_data/strain/imperfect_solution/expected_analysis.json

@@ -0,0 +1,2 @@
+{"summary":"Check the comments for things to fix. 🛠 ","comments":[{"comment":"elm.strain.do_not_use_filter","type":"essential","params":{}}]}


Noted! I didn't know about that line inconfig.json, thank you.

I don't think I can request a review from @mpizenberg or @ceddlyburge on website-copy, they are not on the list. How can we add them in there? We will need to approve analyzer comments in the future. Maybe in the mean time you could approve it?

test_data/strain/perfect_solution/expected_analysis.json

ceddlyburge · 2022-03-02T08:39:35Z

Dockerfile

@@ -0,0 +1,53 @@
+FROM node:lts-alpine AS builder


I use a tool called HadoLint in VsCode to lint Dokerfiles. It has 3 warning messages and 1 info message for this file. There is nothing drastic but it is probably worth taking a look. If you can't run the tool yourself let me know and I can drop the messages in here or we can have a call.

I managed to install it and make it happy :)
Thank you for the recommendation.

README.md

ceddlyburge · 2022-03-02T08:46:52Z

bin/check_files.sh

@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/bin/sh


Is this part of the smoke test? It might be worth making this obvious in some way if so.

Co-authored-by: Cedd Burge <ceddlyburge@users.noreply.github.com>

jiegillet · 2022-03-06T02:07:30Z

I know I was told not to worry, but I modified run.sh and the smoke test so that it would run in read-only (but still writing in /tmp).

Is there anything left to do before generating the secrets and activating the analyzer in the Elm config file?

mpizenberg · 2022-03-06T08:15:51Z

I know I was told not to worry, but I modified run.sh and the smoke test so that it would run in read-only

Haha It's like you just climbed a mountain, you have a great view, but there is this last spot, 50m away, and your journey would not be complete until you make sure the view isn't better from up there.

If I have a small nitpick (without checking in detail all the PR), why are some files are with an eol at the eof while some do not have an eol? I think the posix standard ask that we add an eol as the last char of a file. If you use VSCode it should be this setting: "files.insertFinalNewline": true

jiegillet · 2022-03-06T11:54:50Z

Haha yeah, it's that kind of feeling. And when you reach this other perfect peak, you spot another one :)

Thank you for the EOL tip, it is something that I never really paid attention to, I do use VSCode and I've enabled the setting.

ErikSchierboom · 2022-03-07T18:57:29Z

.github/workflows/docker.yml

@@ -0,0 +1,21 @@
+name: Deploy


This file is now a duplicate of the deploy.yml file and can be removed.

ErikSchierboom · 2022-03-08T09:04:14Z

From my point of view, this is ready to be merged!

ceddlyburge · 2022-03-08T09:20:24Z

me too :)

jiegillet added 8 commits March 1, 2022 23:07

Build and run in docker

80661a3

Fix solution_cache.tar path

Run tests in docker

0918c0b

CI: push docker image (copied from Elixir)

0e694d4

CI: run elm-test, elm-format, tests in docker

54e0e15

Add elm-review on repo, with CI check

14f54cf

Add expected_analysis files

b618415

Fail if expected_analysis.json is not committed

aa85892

Add README

44da630

jiegillet requested a review from a team as a code owner March 1, 2022 14:26

jiegillet added 2 commits March 1, 2022 23:28

Typo in README

8cf7863

CODEOWNER restrict admin to the .github folder

c4464fd

ErikSchierboom reviewed Mar 1, 2022

View reviewed changes

Revert "CODEOWNER restrict admin to the .github folder"

a72b483

This reverts commit c4464fd.

jiegillet added 2 commits March 2, 2022 10:20

SHA instead of version number

4194688

Move copy down the line

ad0d79f

jiegillet commented Mar 2, 2022

View reviewed changes

ceddlyburge approved these changes Mar 2, 2022

View reviewed changes

ErikSchierboom approved these changes Mar 2, 2022

View reviewed changes

jiegillet and others added 7 commits March 2, 2022 19:43

Grammar

028b6a2

Co-authored-by: Cedd Burge <ceddlyburge@users.noreply.github.com>

Apply HadoLint tips

a6411cd

Add comments to check_files.sh

916a982

curl 7.81 not available

9216dbf

Most precise version of curl

b693c38

update deployment CI

68226fa

Do not ignore failure in smoke test

8739b91

jiegillet mentioned this pull request Mar 5, 2022

Running smoke tests in docker exercism/elixir-analyzer#266

Merged

match production flags

68cc357

jiegillet force-pushed the jie-analyzer-docker branch from f77ae93 to 68cc357 Compare March 6, 2022 01:57

run smoke test in parallel

9d30b14

Add newlines at EOF

a5b1ad9

ErikSchierboom reviewed Mar 7, 2022

View reviewed changes

ErikSchierboom added the x:size/massive Massive amount of work label Mar 7, 2022

Remove duplicate docker.yml

8c6ceed

ceddlyburge merged commit 198a315 into exercism:main Mar 8, 2022

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Closed

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

		@@ -0,0 +1,2 @@
		{"summary":"Check the comments for things to fix. 🛠 ","comments":[{"comment":"elm.strain.do_not_use_filter","type":"essential","params":{}}]}

Uh oh!

Add docker integration, CI, and internal elm-review #27

Add docker integration, CI, and internal elm-review #27

Uh oh!

Conversation

jiegillet commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Docker

CI

elm-review

README

Going live

Maintaining the repo

Uh oh!

ErikSchierboom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ceddlyburge commented Mar 1, 2022

Uh oh!

mpizenberg commented Mar 1, 2022

Uh oh!

jiegillet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiegillet commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpizenberg commented Mar 6, 2022

Uh oh!

jiegillet commented Mar 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ceddlyburge commented Mar 8, 2022

Uh oh!

Add docker integration, CI, and internal `elm-review` #27

Add docker integration, CI, and internal `elm-review` #27

jiegillet commented Mar 1, 2022 •

edited

Loading

`elm-review`

jiegillet commented Mar 6, 2022 •

edited

Loading

ErikSchierboom commented Mar 8, 2022 •

edited

Loading