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

CI on Github Actions #719

Merged
merged 7 commits into from Mar 8, 2024
Merged

CI on Github Actions #719

merged 7 commits into from Mar 8, 2024

Conversation

matiasgarciaisaia
Copy link
Member

@matiasgarciaisaia matiasgarciaisaia commented Mar 6, 2024

This PR moves the CI from CircleCI to Github Actions.

The only breaking change we introduce is that the instedd/planwise-mapserver images are now tagged VERSION-mapserver and VERSION-mapcache instead of mapcache-VERSION like before.

Update APT's sources with the new archived URL
Also, stop sharing the host's `lein` directory, and set the platform to
be AMD64-only - we don't support ARM yet.
The previous version relied in external scripts having already built the
correct version of the binaries. Now running `docker build` should be
enough to properly build an image of the app.

There are optimisations to apply, for sure.
@matiasgarciaisaia matiasgarciaisaia marked this pull request as ready for review March 6, 2024 20:32
Copy link
Member

@ggiraldez ggiraldez left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a couple of minor comments.

Comment on lines +16 to +17
dockerBuildAndPush -d mapserver -s "-mapserver" -t "-mapserver" -o "-f mapserver/Dockerfile.mapserver"
dockerBuildAndPush -d mapserver -s "-mapserver" -t "-mapcache" -o "-f mapserver/Dockerfile.mapcache"
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to add the tag suffix as well as the image suffix?

@@ -2,9 +2,6 @@ version: '2'

services:
app:
volumes:
- .:/app
- ~/cache/.m2:/root/.m2
Copy link
Member

Choose a reason for hiding this comment

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

The Maven cache would be good to restore at some point. The overhead of downloading the same dependencies over and over is high.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should eventually split the image in two - one with the base dependencies, and the other one with that PLUS the app's code.

Comment on lines -15 to -16
MAPCACHE_TAG=instedd/planwise-mapserver:mapcache-$TAG
MAPSERVER_TAG=instedd/planwise-mapserver:mapserver-$TAG
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see why you added the tag suffix above. I think in this case the TAG was not meant to be a versioning tag, but a content tag. Ie. at some point we would generate the mapserver image with some regional TIFFs included. But I don't think we've used them in that way for a very long time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting.

I see in Docker Hub we have some tag kenya-mapserver-stable, but I also see mapcache-latest, mapcache-master & mapcache-stable (created a few years ago).

I mean, I really am not sure about what these things are or do, so I'll trust your judgment here. I'm just replicating the CI we had.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait. I didn't realize that we didn't have a different repo for mapcache. That's what the tag is being used for. I'd go for a different Docker repo though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree on that. I'll leave that for later, though.

@matiasgarciaisaia matiasgarciaisaia merged commit ae263cc into master Mar 8, 2024
4 checks passed
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