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

Add EventStore image #5327

Closed

Conversation

riccardone
Copy link

@riccardone riccardone commented Jan 22, 2019

Added EventStore for official docker image.
The image is Open Source.

Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

  • associated with or contacted upstream?
  • does it fit into one of the common categories? ("service", "language stack", "base distribution")
  • is it reasonably popular, or does it solve a particular use case well?
  • does a documentation PR exist? (should be reviewed and merged at roughly the same time so that we don't have an empty image page on the Hub for very long)
  • dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?
  • 2+ dockerization review?
  • existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?)
  • if FROM scratch, tarballs only exist in a single commit within the associated history?
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

Copy link
Contributor

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Purely technical review to help taking off some load from the Docker Library team. I am not part of the Docker Library Team, so take this review with a grain of salt and possibly wait for a general “ok” before taking time to fix the image.

That said (reviewing the master of the repository, as the commit is invalid):


GitRepo: https://github.com/EventStore/eventstore-docker

Tags: release-4.1.1-hotfix1
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to have latest, 4 and 4.1 aliases.

GitRepo: https://github.com/EventStore/eventstore-docker

Tags: release-4.1.1-hotfix1
GitCommit: cd0eafec71baffbcc0b9a4bb7c58e996cf290d34
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it appears this is a commit on the main EventStore repo, not the eventstore-docker repo: EventStore/EventStore@cd0eafe

I'd recommend giving https://github.com/docker-library/official-images#contributing-to-the-standard-library a read-through (and @TimWolla's other comments were definitely spot-on ❤️). There appear to be quite a few things noted in the docs there that aren't adhered to here.

I'd also add that apt-get autoremove isn't likely to do anything (since you don't have any packages being removed here), and apt-get clean is done automatically by the base image itself. Also, if your install process is leaving extra files in /tmp or /var/tmp, I'd highly recommend trying to figure out why (since installing a Debian package shouldn't typically be leaving trash in those places).

@yosifkit
Copy link
Member

Gif to brighten the day:

Apologies for the long delay. 🙇‍♂️

Is this PR still a desirable addition? I see that there was a PR to adjust the Dockerfile, but it seems to have stalled (EventStore/eventstore-docker#61). How can we help?

@thefringeninja
Copy link

We have moved our Dockerfile to the main repository: https://github.com/EventStore/EventStore/blob/master/Dockerfile

I think we have addressed most concerns (except HEALTHCHECK) by completely changing the way it is built, but I don't have permission to push to our fork. Once I get them I will update this PR.

@yosifkit
Copy link
Member

Again, apologies for the delays. 🙇‍♂️ New images unfortunately get the least priority but often take the most time to review, so they often fall by the wayside.

Unfortunately moving the Dockerfile (while useful for local development) doesn't work well as the target for official-images. Since the Dockerfile uses COPY ./src ., it essentially means the entire EventStore source code is now part of the Docker build context, and thus becomes part of the reviewed artifacts. (see diff-pr.sh for how we generate our diff; it is basically taking all docker build contexts from the starting library/EventStore file and diffing them to the new contexts from the PR).

Regarding multi-stage builds for official-images, see our FAQ entry on multi-stage builds and the beginning of this comment (docker-library/ghost#209) for my current thoughts.


I can continue to review the Dockerfile in EventStore/EventStore, but it doesn't seem to have the same goal as a Dockerfile for an official-image (being geared toward developer usage). Since you have releases available as .deb, .tar.gz and Mono, it would make sense to just use those (https://github.com/docker-library/faq/#why-do-so-many-official-images-build-from-source).

As for review on the EventStore/eventstore-docker Dockerfile, the initial thing I'd point out (besides the reviews above) would be that curl | bash is not acceptable and should have some sort of verification of the download before being used (https://github.com/docker-library/official-images/tree/b7ec40a45da14cd6a2c2a251f5b56e59dbc6cc25#security), although in this case the 2-3 things it does should probably just be inlined instead since it appears to be just for setting up /etc/apt/sources.list?

@github-actions
Copy link

github-actions bot commented May 5, 2020

Diff for d779a78:
failed fetching repo "eventstore"
unable to find a manifest named "eventstore" (in "/tmp/tmp.S8InPcHo6O/oi/library" or as a remote URL)
failed fetching repo "eventstore"
cannot parse manifest in either format:
RFC 2822 error: Tags "" has invalid GitCommit (must be a commit, not a tag or ref): ""
Line-based error: manifest line missing '@': GitRepo: https://github.com/EventStore/eventstore-docker

@tianon
Copy link
Member

tianon commented Jun 24, 2020

Closing this for now -- when the repository is updated with the requested changes and you're ready to recommit to maintaining this image, please reopen! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants