Move from Travis to Github Actions

[leonardehrenfried]

This PR adds a skeletal nominatim integration test that uses a docker image (https://github.com/stadtnavi/nominatim-docker/tree/master/latest) which import Monaco, connects to it and imports a single document.

There is a lot of room for improvement, but I would like to submit this to review to figure out if it's going into the right direction.

Test output is here: https://github.com/mfdz/photon/runs/758004572

Note: I took the liberty to remove travis.yml. I hope this is ok.

Open question: is Java 8 still supported and should the test run with it as well as Java 11?

[lonvia]

I totally forgot that we have already a similar attempt at #424. What we did there was to produce a small database dump which was simply loaded with pg_restore. The advantage is that you can save the whole docker setup. The fact that it is an externally maintained project, is a little bit scary because they can basically just break our integration tests. I'm not saying that anybody plans to do it but it is a risk that needs to be considered. On the other hand, the docker setup has the advantage that we always test against the latest Nominatim and find breakages in compatibility.

Regarding Java version: we currently support Java 8+. I don't think anybody has tested against anything more recent than Java 11. It's definitely good to run against multiple versions.

Is there something special that needs doing to enable github Actions? I assumed that just adding .github/workflows would do the trick?

[leonardehrenfried]

I totally forgot that we have already a similar attempt at #424. What we did there was to produce a small database dump which was simply loaded with pg_restore. The advantage is that you can save the whole docker setup. The fact that it is an externally maintained project, is a little bit scary because they can basically just break our integration tests. I'm not saying that anybody plans to do it but it is a risk that needs to be considered. On the other hand, the docker setup has the advantage that we always test against the latest Nominatim and find breakages in compatibility.

I'm fine with following along with whatever you find most suitable. If that means a DB dump, that's fine by me. If you want to have more control over the docker image I'm ok with adding a copy in this or any other repo.

Regarding Java version: we currently support Java 8+. I don't think anybody has tested against anything more recent than Java 11. It's definitely good to run against multiple versions.

Ok, understood. The question mas more about whether Java 8 compatibility is required given that it's been superseded as an LTS for some years. (Side note: I'd love to move to Java 11 so I can use the extra features.)

Is there something special that needs doing to enable github Actions? I assumed that just adding .github/workflows would do the trick?

No, put the file there and it just works™. That's one of the reasons I prefer it over Travis as a fork will have same CI without having to enable anything.

[simonpoole]

@leonardehrenfried my 2 cents: there is no reason to suspect that using Java 8 as the coding standard would cause any issues with later Java variants. And given the nature of photon I'm not quite sure of any benefits of Java 11 in any case (obviously the driving force for bumping this in the end could well be the dependencies).

But there is a clear (and rather regular) migration task that you could give a look if you feel up to it, updating to a current ES version. We are currently 2 major versions behind which would seem bad (iirc we are talking about multiple years behind now). And I'm not claiming that I even know what updating will involve.

[lonvia]

Can we please have the discussion on ES in #325. It's complicated.

[leonardehrenfried]

@leonardehrenfried my 2 cents: there is no reason to suspect that using Java 8 as the coding standard would cause any issues with later Java variants. And given the nature of photon I'm not quite sure of any benefits of Java 11 in any case (obviously the driving force for bumping this in the end could well be the dependencies).

I have one clear benefit: After having written Scala for a few years, I'm sick of writing the same type over and over again. Java 11 has local variable inference so

Map<Foo,Bar> map = new HashMap<Foo,Bar>();

becomes

var map = new HashMap<Foo,Bar>();

Perhaps this conciseness is what some Java developers dislike but to me this is a very nice improvement. Sounds silly, but it really lessens (my) cognitive load when writing Java.

But there is a clear (and rather regular) migration task that you could give a look if you feel up to it, updating to a current ES version. We are currently 2 major versions behind which would seem bad (iirc we are talking about multiple years behind now). And I'm not claiming that I even know what updating will involve.

Sounds interesting but would it no be a good idea to have an automated integration test so this becomes even possible?

[lonvia]

The big advantage of the dumps is that they can also be used locally by developers. So, let's go with that for the moment. It's also fairly easy to test updates with the same dump. Just run psql -d nominatim -c 'UPDATE placex SET indexed_status = 2 limit 100' and then run the photon update. I'll have a look how up-to-date the dump is on the graphhopper server.

No, put the file there and it just works™. That's one of the reasons I prefer it over Travis as a fork will have same CI without having to enable anything.

I would have expected to see results for this PR but maybe it needs to be live on master first.

Regarding the Java version, I'd also like to defer that discussion to another issue.

[leonardehrenfried]

I hate to sound like a sore loser but the docker setup can also be used locally. But lets not get distracted.

If I understand you correctly then the right way forward is reviving #424, correct?

[lonvia]

I freely admit that I'm a grumpy old developer when it comes to docker. ;)

More seriously, docker and more importantly the nominatim run inside the container adds quite a few moving parts that are just boilerplate for what we actually want to tests. It just seems like a good idea to remove them from the equation. The dump also provides a stable environment, so we can even test for particular data from the dump without needing to worry that the data might be changed or deleted by an OSM user.

I understand that users might prefer to use docker locally instead of worrying about how to install postgresql. We could in a later step provide a docker container with a postgresql preloaded with the test data for them.

So, long story short: yes, reviving #424 would be nice. It might even be a good idea to have separate steps: first move tests to Github actions, second add the #424 stuff.

[leonardehrenfried]

So, as suggested by @lonvia I've removed everything to do with integration tests from this PR and simply moved from Travis to Github Actions.

The test output is here: https://github.com/mfdz/photon/runs/789239777?check_suite_focus=true

In another one I will try to bring back #284

By the way:

curl --head http://download1.graphhopper.com/public/photon-test-data/photon-test.dmp
HTTP/1.1 200 OK
Server: nginx
Date: Fri, 19 Jun 2020 19:22:51 GMT
Content-Type: application/octet-stream
Content-Length: 30948923
Last-Modified: Fri, 14 Jun 2019 16:20:12 GMT
Connection: keep-alive
ETag: "5d03c93c-1d83e3b"
Accept-Ranges: bytes

It seems that the dump was last updated a year ago.

[lonvia]

Shouldn't that be push + pull_request?

[leonardehrenfried]

That's actually not necessary as you need a push to create a PR. Unless you really do want to run the tests a second time when someone clicks the PR button. Additionally you'd want it to run on the master branch as well, correct?

On Sat, 20 Jun 2020, at 22:38, Sarah Hoffmann wrote: ***@***.**** commented on this pull request. In .github/workflows/ci.yml <#471 (comment)>: > @@ -0,0 +1,33 @@

+name: "Continuous Integration" + +on: + push:

…

Shouldn't that be push + pull_request? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#471 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABE6MUTHRTT4QZA3FHKII3RXUM35ANCNFSM4N2MZHDQ>.

[lonvia]

It should run on pushes to master and for every PR (and on additional pushes to it). That's also the policy we have for Travis right now.

With 'push' you only get checks for pushes to branches in this repo, while a PR branch usually exists on a different repo. So you need 'pull_request' to see the check results here in the repo. At least that's how I understand the docs. It would also explain why the checks don't show up for this PR.

[leonardehrenfried]

Hmm, never thought about it that way. It's a really good point. I'll correct this.

…

On Sun, 21 Jun 2020, at 09:33, Sarah Hoffmann wrote: It should run on pushes to master and for every PR (and on additional pushes to it). That's also the policy we have for Travis right now. With 'push' you only get checks for pushes to branches in this repo, while a PR branch usually exists on a different repo. So you need 'pull_request' to see the check results here in the repo. At least that's how I understand the docs. It would also explain why the checks don't show up for this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#471 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABE6MUMH3AEOB5SEIUKJ5LRXWZT7ANCNFSM4N2MZHDQ>.

[leonardehrenfried]

Now with pull_request in the job definition: https://github.com/mfdz/photon/runs/794331567

[lonvia]

Still no message github in this repo. Could you try without the branch: master definition?

[leonardehrenfried]

I have now. I think you will only see the notifications once you have merged it.

[lonvia]

Oh well, let's try it then.

[leonardehrenfried]

It worked: https://github.com/komoot/photon/runs/802706045