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

Single docker image for minor release #55

Merged

Conversation

sanak
Copy link
Member

@sanak sanak commented Nov 23, 2022

Supports #53

@cayetanobv @krashish8 (CC: @cvvergara, @smellman)

Changes proposed in this pull request:

I confirmed that make build generated all docker images without error in about 4 hours on my Ubuntu 22.04 amd64 desktop PC.

Confirmation point

  1. Isn't it necessary to keep the past versions folders in ./backup folder ?
  2. About pgRouting versions,
    • Is it enough to support recent 3.3 and 3.4 versions ?
    • Isn't it necessary to support develop branch, because it seems to be a bit out of date from main branch ?

Original (old) comment Currently working PR for #53

I still haven't tested make update at all, so this is still my trial PR.

About fetching latest patch version from pgRouting major.minor versions, I referred docker-library/redmine repository side, because postgis/docker-postgis side seems to depend on debian apt package release.
https://github.com/docker-library/redmine/blob/master/update.sh


  • 2022/11/23: I will try minimum test at tomorrow.
  • 2022/11/25: I will add version.txt to store full version for docker tag name's version part (like 15-3.3-3.4.1 in 15-3.3-3.4 folder ,etc.)

@sanak sanak self-assigned this Nov 23, 2022
@sanak sanak changed the title (Local) Single docker image for minor release Single docker image for minor release Nov 23, 2022
@sanak sanak changed the title Single docker image for minor release WIP: Single docker image for minor release Nov 23, 2022
@sanak sanak force-pushed the single-docker-image-for-minor-release branch from df6b42e to 0f71ac5 Compare November 24, 2022 16:58
@sanak sanak force-pushed the single-docker-image-for-minor-release branch from 9bb9c69 to 71ed798 Compare November 26, 2022 01:02
@sanak sanak marked this pull request as ready for review November 26, 2022 04:16
@sanak sanak changed the title WIP: Single docker image for minor release Single docker image for minor release Nov 26, 2022
Copy link
Member

@cayetanobv cayetanobv left a comment

Choose a reason for hiding this comment

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

Great work @sanak ! I've tested all your new code (compiling a new image and using the makefile) and it's working fine. If somebody from @pgRouting/admins hasn't something more to add or fix on this PR, we can proceed in the next few days with the merge. Thanks.

@cayetanobv
Copy link
Member

Can we close this PR #49 @krashish8 ? I think this was already solved here.

Copy link
Member

@krashish8 krashish8 left a comment

Choose a reason for hiding this comment

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

Overall, this looks great! Thanks, @sanak! 🎉
We can go ahead and merge this PR. All the docker images were built successfully.
(Got a bit busy last week, so I could not review the PR at that time)

Left a small comment, but it can be dealt with later in another PR, if needed.

@@ -0,0 +1,45 @@
FROM pgrouting/pgrouting:11-3.3-3.3.5
Copy link
Member

Choose a reason for hiding this comment

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

Is the pgrouting docker image tagged as pgrouting/pgrouting:11-3.3-3.3.5 or pgrouting/pgrouting:11-3.3-3.3?

If it is tagged as the latter one, then we should update the Osm2pgRouting Dockerfile such that it references the correct pgRouting docker image version (that does not contain .patch name). Need to update update.sh file to take this into account.

Copy link
Member

Choose a reason for hiding this comment

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

We are storing in the repository only pgrouting major.minor, but we need to compile the exact version to avoid conflicts when you upgrade the docker image in your server (or local environment). Anyway we continue discussing that in another PR or issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@krashish8 Thanks for comments and approval!

Is the pgrouting docker image tagged as pgrouting/pgrouting:11-3.3-3.3.5 or pgrouting/pgrouting:11-3.3-3.3?

If it is tagged as the latter one, then we should update the Osm2pgRouting Dockerfile such that it references the correct pgRouting docker image version (that does not contain .patch name). Need to update update.sh file to take this into account.

It is tagged as the former one (pgrouting/pgrouting:11-3.3-3.3.5) from version.txt contents.

In the following Makefile changes, docker image tag will be created as follows.

  1. Create tag from version.txt contents (ex. 15-3.3-3.4.1).
  2. If $1 (=folder name (ex. 15-3.3-3.4)) doesn't equal version.txt contents (ex. 15-3.3-3.4.1), create another tag (alias) as 15-3.3-3.4.
- 	docker build --pull -t $(REPO_NAME)/$(IMAGE_NAME):$(shell echo $1) $1
- 	docker build -t $(REPO_NAME)/$(IMAGE_NAME)-extra:$(shell echo $1) $1/extra
+ 	docker build --pull -t $(REPO_NAME)/$(IMAGE_NAME):$(shell cat $1/version.txt) $1
+ 	docker build -t $(REPO_NAME)/$(IMAGE_NAME)-extra:$(shell cat $1/version.txt) $1/extra
+ 	if [ "$(shell echo $1)" != "$(shell cat $1/version.txt)" ]; then\
+ 		docker tag $(REPO_NAME)/$(IMAGE_NAME):$(shell cat $1/version.txt) $(REPO_NAME)/$(IMAGE_NAME):$(shell echo $1);\
+ 		docker tag $(REPO_NAME)/$(IMAGE_NAME)-extra:$(shell cat $1/version.txt) $(REPO_NAME)/$(IMAGE_NAME)-extra:$(shell echo $1);\
+ 	fi

So, after executing make build, created docker images will be as follows.

$ docker image ls
REPOSITORY                  TAG              IMAGE ID       CREATED        SIZE
pgrouting/pgrouting-extra   15-3.3-main      edc542164c5b   7 hours ago    654MB
pgrouting/pgrouting         15-3.3-main      6798de41297c   7 hours ago    623MB
pgrouting/pgrouting-extra   15-3.3-develop   c44cf12cfe7a   7 hours ago    654MB
pgrouting/pgrouting         15-3.3-develop   258c78de7f1c   7 hours ago    623MB
pgrouting/pgrouting-extra   15-3.3-3.4       e58bdc8a471b   7 hours ago    654MB
pgrouting/pgrouting-extra   15-3.3-3.4.1     e58bdc8a471b   7 hours ago    654MB
pgrouting/pgrouting         15-3.3-3.4       af0216624721   7 hours ago    623MB
pgrouting/pgrouting         15-3.3-3.4.1     af0216624721   7 hours ago    623MB
:

@krashish8
Copy link
Member

krashish8 commented Dec 2, 2022

Answering your queries (based on my own point of view):

Isn't it necessary to keep the past versions folders in ./backup folder?

I think it is not necessary to do that. We use git for the same purpose - anyone who wants to use these files can go back in history any time using git. If we are no longer maintaining these docker files, maybe we should not keep them. Anyway, there is no issue with keeping these files in the backup folder, so we are good for now.

Is it enough to support recent 3.3 and 3.4 versions?

I think yes, because the last release of 3.2 was last year, and we are no longer maintaining the older releases in pgrouting, aka no bug fixes. So, it is okay if we don't maintain the older releases docker container.

Isn't it necessary to support develop branch, because it seems to be a bit out of date from main branch?

Yes, I think we can support both main and develop branches - useful for those users who want to test the live "development" code but don't want to build from scratch - they can use the Dockerfile for main/develop to use the non-released code. I think this has been taken care of in this PR.

For questions 1 and 2 - I would also add that if we push Docker images in Dockerhub (or any such container library), it will contain the older releases, as normally we won't be deleting those, and anyone wanting to use them could pull them using Dockerhub. We won't be just maintaining the older Dockerfiles in this GitHub repo.

@krashish8
Copy link
Member

Can we close this PR #49 @krashish8 ? I think this was already solved here.

Yes, @cayetanobv. I have closed that PR.

@cayetanobv
Copy link
Member

Thanks for your review @krashish8 . I'm going to merge the PR

@cayetanobv cayetanobv merged commit dd6672d into pgRouting:master Dec 2, 2022
@sanak
Copy link
Member Author

sanak commented Dec 3, 2022

@cayetanobv Thanks for merging this PR! 🙇‍♂️

@krashish8 Thanks for answering my queries!
I will create another PR which deletes ./backup folder, later.

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.

3 participants