[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > Please make a ticket in JIRA as a reminder for that and assign it a fix version of 3.4.0 so we don't forget. Done: [TINKERPOP-1980](https://issues.apache.org/jira/browse/TINKERPOP-1980) And thank you, @spmallette, for the thorough review and for doing the first deployment with the fixes you had to do to get it actually working. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 Here's what I just did: * Confirmed that SNAPSHOT does not deploy images. * Confirmed that 3.2.10-rc1 deploys 3.2.10-rc1 only - don't see 3.2 up there any more I guess that's as much as I can test for now until we release. I will say that so far these tweaks haven't been painful so if we have to sort something out the next time we do a release it shouldn't be too challenging to get it done. > Once we release 3.4.0 we need to remove the latest tagging from the tp33 branch so that 3.4.0 will be latest. Please make a ticket in JIRA as a reminder for that and assign it a fix version of 3.4.0 so we don't forget. Other than that, I think this is all good now. Feel free to close the TINKERPOP-1897 in JIRA now. Stellar job on this @FlorianHockmann - thanks for sticking with it to get it to the end. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 Just pushed two CTR commits to hopefully get the tags right: [aac534e11](https://github.com/apache/tinkerpop/commit/aac534e11fa406bedc61dbc060d06740b898e170) adds a check to only tag Docker images with `3.2` when the version is not a prerelease version, so not `3.2.10-SOMETHING`. [d4faa66](https://github.com/apache/tinkerpop/commit/d4faa6605495e072d65578682d2ed5fd12bdb5ad) adds the `latest` tag on `tp33` branch and on `master`. Once we release 3.4.0 we need to remove the `latest` tagging from the `tp33` branch so that `3.4.0` will be latest. (I don't see a way to automate this so we really have to remember to do it manually before the release of 3.4.0.) This should result in the following tags: * tp32 release candidates: 3.2.10-rc1 * tp32 releases: 3.2.10, 3.2 * tp33 release candidates: 3.3.4-rc1 * tp33 releases: 3.3.4, 3.3, latest To give a short explanation of why this tagging makes sense in my opinion: * Users who always want to have the newest version that is still stable can use the `latest` tag. This is also implicitly used when you do `docker run tinkerpop/gremlin-console`. So `latest` should always be available and always point to something stable. * Users who to stay on `tp32` for example because their graph provider doesn't support 3.3.0 yet can use the `3.2` tag and will always get the latest stable version of that branch. (Same applies of course to the `3.3` tag.) * All other users will select a version explicitly. This could either be a stable version like `3.2.10` or a release candidate when they want to check out new features earlier, but they should always get a stable version when they choose a tag like `3.2.10`. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 Updated the DockerHub descriptions a bit - looks prettier now: https://hub.docker.com/r/tinkerpop/gremlin-server/ https://hub.docker.com/r/tinkerpop/gremlin-console/ ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 so - that changes works. we can tweak it now if necessary now that we know it works: https://github.com/apache/tinkerpop/commit/144c69801de8135ac04663da4063260378c71336 > But do we want to add the 3.2 tag for a 3.2.10-rc1 image? ... IMO release candidate images should only have their full version as a Docker tag and not something like 3.2 as that usually implies a stable version. As i don't really use docker, I don't know what people tend to expect when they use it. Can you tweak the poms accordingly please with some CTR commits and I can try to deploy again? ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > I believe that aspect of this works fine. Nothing deploys when the version is a SNAPSHOT. But do we want to add the `3.2` tag for a `3.2.10-rc1` image? That means that users who specify `3.2` and were previously on an official release `3.2.9` that they now get a release candidate version. IMO release candidate images should only have their full version as a Docker tag and not something like `3.2` as that usually implies a stable version. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 > Yes and the 3.2 tag should only exist for non-SNAPSHOT versions, I believe that aspect of this works fine. Nothing deploys when the version is a SNAPSHOT. > But maybe we can add the tagging as a separate execution just for this tag? it appears you need multiple `push` executions: https://github.com/spotify/dockerfile-maven/issues/10 i'm trying it now. > So, yes, I guess we have to edit the descriptions manually in the web interface. ok - not a big deal. i can fix that. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > Separately, do I need to edit the repository short/long description manually? can that not be automatically done on deploy using content from our github repo? I'm afraid that only works for automated builds which we can't use as it would have to work with just a Dockerfile and all other files already being present. Since we need to build the artifacts first with Maven this seems to be no option for us. So, yes, I guess we have to edit the descriptions manually in the web interface. I just [CTRed a commit](https://github.com/apache/tinkerpop/commit/1c3f34f52b2651897aa2fe89527cae8d2357dcc6) to tp32 that made the two `docker-entrypoint.sh` scripts executable as the Docker images contained versions of them that were actually not executable making the images basically unusable. I assumed that git would preserve the executable bit as I set it locally, but apparently it was necessary to set it manually again :-/ ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue:
https://github.com/apache/tinkerpop/pull/802
> I only see the 3.2 tag - where the 3.2.10-rc1? shouldn't that be up there
too?
Yes and the `3.2` tag should only exist for non-SNAPSHOT versions, but I
guess that we didn't think about release candidate versions when we added that
check:
```xml
only.when.is.snapshot.used
${project.version}
.*-SNAPSHOT
```
https://github.com/apache/tinkerpop/blob/tp32/pom.xml#L283
Regarding the full version (3.2.10-rc1), my assumption was that this is
already handled by:
```xml
${project.version}
```
https://github.com/apache/tinkerpop/blob/tp32/gremlin-console/pom.xml#L369
and I'm pretty sure that it worked like that in my tests.
But maybe we can add the tagging as a separate `execution` just for this
tag?
---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 I've published but look at what is up there: https://hub.docker.com/u/tinkerpop/ I only see the 3.2 tag - where the 3.2.10-rc1? shouldn't that be up there too? ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > I also just pushed a minor update to make the command more like our others with -DdockerImages rather than explicitly using the -P directly which we typically dont' do. Also disabled the deployment of java artifacts since docker deployments will occur out of band and we won't want those re-deployed when we go to deploy to docker. Makes sense. I just rebased this on `tp32` as there was a small merge conflict in the CHANGELOG and will merge it now. Really excited to see TinkerPop's first own Docker images ð ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 @FlorianHockmann I think you can merge this. I will do the deploy afterwards when I can tag the rc1 properly. If something goes wrong we can just CTR in some fixes. I'm feeling pretty confident about how this is going to work now after playing with things locally a bit. I also just pushed a minor update to make the command more like our others with `-DdockerImages` rather than explicitly using the `-P` directly which we typically dont' do. Also disabled the deployment of java artifacts since docker deployments will occur out of band and we won't want those re-deployed when we go to deploy to docker. When I see this merged, I will do the rc1 deployment. This was a nice PR and I think people will really appreciate this. Thanks! ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 ah - yea, i did, but forgot. i think others should be free to vote now though. let's just hold the merge until i get a minute to test that. i think i can get to that this week, though i guess i need to run it by the dev list first. will start a thread there now. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 @spmallette you now voted +1 on this but didn't you want to push release candidates of the Docker images during the review? ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 I made a minor comment on the last dev doc update to the pre-flight check. Other than that: VOTE +1 ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 No, you're right. That is something that is very unlikely to occur and I can't think of other (more likely) reasons where the build of the docker images could fail as they really only copy the packaged artefacts together with the `docker-entrypoint.sh` scripts into the image. It is more likely that the built images themselves stop working for some reason, but we couldn't find that during the maven build anyway. So making it a habit to check the Docker images before a release seems to be the better option to me. I'll push a commit to remove the `.docker` file option and add a note to the docs section you linked. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 > detect when changes break the Docker images, e.g., when the naming of the zip archives or their location changes That's fair, though that specific example seems really unlikely to occur to me at this point and if it did happen the fix is relatively quick (i would guess) so even if it happened during code freeze week we wouldn't have a big problem. We could even add a point to the pre-flight check to make sure we generate images during code freeze - http://tinkerpop.apache.org/docs/current/dev/developer/#_pre_flight_check Can you think of any other reasons it would fail to build based on more "day-to-day" changes? ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > what is the advantage to building these images if a .docker file is present? A reason to keep this option is that it makes it easier to detect when changes break the Docker images, e.g., when the naming of the zip archives or their location changes. For those cases, it would be good to build the images with every Maven build (assuming that Docker is installed locally). The build should also basically be a no-op when nothing changed about the Docker images, although the build time slightly increases from 33s to 51s for `gremlin-server` on my system without and with activated `docker-images` profile and the image already present (from my comment on 1 Mar). I expanded the dev docs for the two aspects you mentioned (Docker Hub account and release process for the Docker images). ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 Rebased and removed the hard-coded version. The major and minor version are now parsed with the `build-helper-maven-plugin` and added as tags to the image when it's not a `SNAPSHOT` version. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 @FlorianHockmann could you rebase this please? maybe we can try to get this polished up now. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 > move that entire configuration to the root pom.xml Good point @spmallette, I just pushed a commit for that and also rebased the PR. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 Since the `build-helper-maven-plugin` configuration is identical in both the gremlin-server/gremlin-console pom.xml files and the "SNAPSHOT detection" would be generally useful (i think it has value to the GLVs) i think that you should be able to move that entire configuration to the root pom.xml (where the `build-helper-maven-plugin` is already being used). I'm pretty sure that doing that will make that `only.when.is.snapshot.used` to all sub-modules. I think you can then remove the definition for `build-helper-maven-plugin` from the `` element. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 I'm ok with merging this after the next release. Starting with a release candidate first like we usually do for GLVs makes sense in my opinion. Then we can also deal with things like the description for Docker Hub and more people can try out the images before we do an official release. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 I haven't looked at this in detail yet, but is your intent to see this merged before we release 3.2.8/3.3.2? Personally, I'd like to see it wait until a following release so that we don't need to do any release instruction changes. I think we might even want to try to do a release candidate to docker first to make sure we get the release process right for when we go official. thoughts? ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 Thanks @spmallette, that actually looks like a good solution. I just pushed a commit that uses this solution. This not only allows skipping of `docker push` for SNAPSHOT versions, but I could also enable tagging of the minor version when the version is not SNAPSHOT, e.g., `3.2` for the `tp32` branch. I can also add the `latest` tag on `tp33` either with another PR or while merging from `tp32`. So the _To Do_ from this PR's initial post is now also done. To sum up what happens for the different versions: * `3.y.z-SNAPSHOT`: * Images are just built. * Tag: `3.y.z-SNAPSHOT` * `3.2.z`: * Images are built and pushed. * Tags: `3.2.z` and `3.2` * `3.3.z` (not directly part of this PR): * Images are built and pushed. * Tags: `3.3.z`, `3.3`, and `latest` I also verified the generated docs with `./docker/build.sh -d`. VOTE +1 ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 i saw a few solutions to the issue of skipping when SNAPSHOT - not sure which is best, but here's one: https://stackoverflow.com/a/39139979/1831717 we might actually find usage for this elsewhere in tinkerpop once the process is established. i seem to remember some hacky things we do around this problem, but i don't recall exactly what..maybe i'm making it up :) ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 I just verified my assumption that `docker login` should be enough to get deployment of Docker images working through Maven and added a short description of that to the dev docs. > if i understand you correctly, you're saying that if you push the image to 3.3.2-SNAPSHOT and then make changes and push again, the 3.3.2-SNAPSHOT would update...is that right? if that is the case, then that behavior seems to match the nature of a java "SNAPSHOT" (except that there is no archival of the older SNAPSHOT). Yes, exactly. Changing something about the image means that the ID will change and the tag will be added to the new image. So the old one won't be available any more through Docker Hub as it isn't tagged any more. That way, we only have one image per tag available and it will always be the most recent one. > Either way though, I don't think we need to pollute the TinkerPop dockerhub with SNAPSHOT deployments. I think we should look for ways to disable the docker plugin on deploy if it is a SNAPSHOT version. I agree, I just don't know how this could be done. The plugin [contains `skip` flags](https://github.com/spotify/dockerfile-maven#skip-docker-goals-bound-to-maven-phases), but we have to activate them somehow for SNAPSHOT versions. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 > Otherwise I could also test deployment with this plugin with some test image to a private repo and then write the dev docs. That would be great if you could test things out in private first since that is an option. It would be nice to know that we've worked out all the kinks in a throwaway repo before we start putting things out in public. > It should be enough to do docker login that's good to know - a few words on that in the dev docs should be sufficient so that no one has to go look it up in google. > Now, doing a mvn deploy and therefore docker push for an image that was already pushed shouldn't do anything when the artifacts didn't change that go in that image. if i understand you correctly, you're saying that if you push the image to 3.3.2-SNAPSHOT and then make changes and push again, the 3.3.2-SNAPSHOT would update...is that right? if that is the case, then that behavior seems to match the nature of a java "SNAPSHOT" (except that there is no archival of the older SNAPSHOT). Either way though, I don't think we need to pollute the TinkerPop dockerhub with SNAPSHOT deployments. I think we should look for ways to disable the docker plugin on `deploy` if it is a SNAPSHOT version. I'm not sure that there is a way to do that with maven directly, but I think we can come up with a way to have the effect of not sending SNAPSHOT images to dockerhub. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user FlorianHockmann commented on the issue: https://github.com/apache/tinkerpop/pull/802 I just pushed two commits, one removes the log output as you suggested and the other one was unfortunately necessary on my system to get all tests running with `./docker/build.sh -t -n -i`. It simply increases the timeout for one test in `gremlin-driver` as that was failing consistently for me. As already discussed on the mailing list, I can't build the documentation right now, so I can't test if my changes broke anything there. > So mvn deploy will deploy to where? dockerhub? My understanding of the `dockerfile-maven-plugin` is that it will simply wrap `docker push` for deployment which should default to dockerhub. > If so, could you include something in the dev docs on how you configure your system to allow for that? It should be enough to do [`docker login`](https://docs.docker.com/engine/reference/commandline/login/) once on your system which should ask for the credentials and then store them in the config. After that, deployment through maven should just work. How about we deploy release candidate images once this is merged and then write the release docs when we've actually done one successful deployment? Otherwise I could also test deployment with this plugin with some test image to a private repo and then write the dev docs. > Also, what happens when we do a mvn deploy on a SNAPSHOT? that would be bad right? I think we need to prevent such things from happening somehow. I'm not sure yet about that one yet. It would just result in an image with a tag like `3.2.8-SNAPSHOT` which shouldn't be a problem. The ideal case would probably be that we use unique tags like `3.2.8-SNAPSHOT-abcde` and then tag the latest of these SNAPSHOT images additionally with `3.2.8-SNAPSHOT`, but just having the `3.2.8-SNAPSHOT` tag should also be ok. Now, doing a `mvn deploy` and therefore `docker push` for an image that was already pushed shouldn't do anything when the artifacts didn't change that go in that image. Alternatively, we could also try to disable the deployment phase for this plugin for `SNAPSHOT` versions, but I don't know how we could do that with Maven. > what does adding this extra step do to the the overall build time (when building docker is enabled, of course)? I just checked with my Ubuntu VM that has just 4 cores and 6 GB RAM and got the following times for `mvn install -pl gremlin-server -DskipTests`: * Without docker build enabled: 33s * With docker build enabled and no images present: 2:10 min * With docker build enabled, but the image already present: 51s When no images are present locally, then it has to download and extract the OpenJDK image which is 82 MB big, that took a big part of that time. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 I just reset my docker account password and it turns out that i'm one of the owners on the TinkerPop organization - so that's good. ---
[GitHub] tinkerpop issue #802: Add docker images for console and server TINKERPOP-189...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/802 Thanks for doing this @FlorianHockmann - i'm just dumping my brain of questions and thoughts on this one. So `mvn deploy` will deploy to where? dockerhub? If so, could you include something in the dev docs on how you configure your system to allow for that? i assume we would want it to deploy here: https://hub.docker.com/u/tinkerpop/ but i can't remember who controls that...i think it's one of us.. Also, what happens when we do a `mvn deploy` on a SNAPSHOT? that would be bad right? I think we need to prevent such things from happening somehow. what does adding this extra step do to the the overall build time (when building docker is enabled, of course)? I like that you made it a separate `` and used the `.docker` pattern we have with `.glv`. Part of this PR will likely need to include updates for the release process as well. ---
