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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include a Dockerfile and instructions. #15

Merged
merged 6 commits into from Aug 27, 2019
Merged

Include a Dockerfile and instructions. #15

merged 6 commits into from Aug 27, 2019

Conversation

@Beanow
Copy link
Member

Beanow commented Aug 16, 2019

Along the same lines of sourcecred/sourcecred#1288

It's unlikely this repository will remain as a separate docker image for long as it would make sense to integrate it as a plugin that runs during the envisioned sourcecred update. But @vsoch mentioned her goal with using the docker containers is to generate the widgets. So this would allow that.

I've set the entrypoint to /code/bin/contributor-wall-svg.js as this is currently the only type of widget being supported. Probably not a very future-proof CLI, but hey, what else would you expect from v0.0.1-alpha? 馃し鈥嶁檪

Along the same lines of
sourcecred/sourcecred#1288
@Beanow

This comment has been minimized.

Copy link
Member Author

Beanow commented Aug 16, 2019

As mentioned in sourcecred/sourcecred#1288
We may need to rename the images to sourcecred/sourcecred and soucrecred/widgets if we're pushing these to Docker Hub.

.gitignore Outdated
@@ -1 +1,2 @@
node_modules
.token

This comment has been minimized.

Copy link
@decentralion

decentralion Aug 16, 2019

Member

I'd prefer to use env rather than .token files, for the reasons discussed by @wchargin here: sourcecred/sourcecred#1288 (comment)

This comment has been minimized.

Copy link
@Beanow

Beanow Aug 16, 2019

Author Member

Missed that as it was resolved already there. Removed those.

This comment has been minimized.

Copy link
@vsoch

vsoch Aug 16, 2019

Contributor

A comment from sourcecred/sourcecred#1288 that I'll include here too - instead of doing an export it might be advisable to just one off define the variable for each command:

SOURCECRED_GITHUB_TOKEN=YOUR_GITHUB_TOKEN docker run --rm -ti -v sourcecred_data:/data -e SOURCECRED_GITHUB_TOKEN sourcecred load sourcecred/sourcecred

I'm okay with the export (I don't share a system with anyone), but this was suggested for security.

Still testing :)

This comment has been minimized.

Copy link
@vsoch

vsoch Aug 16, 2019

Contributor

okay one quick point of feedback - there isn't any instruction to build the container, so I get to the line where I need sourcecred-widgets and I'm like 馃 huh? We should add this, before it is needed:

Next, build the "sourcecred-widgets" container from the Dockerfile in the repository:

$ docker build -t sourcecred-widgets .

This comment has been minimized.

Copy link
@vsoch

vsoch Aug 16, 2019

Contributor

Also we still need to have the GitHub token, so that should be added to the example command.

This comment has been minimized.

Copy link
@vsoch

vsoch Aug 16, 2019

Contributor

It's so beautiful!!

image

Screenshot of an svg, sorry about that :)

@Beanow when we get this merged, I can do another automated build for it, and then guess what! We can make GitHub actions too! I'm so pumped.

Feedback from #15
@vsoch

This comment has been minimized.

Copy link
Contributor

vsoch commented Aug 16, 2019

Oh let me test! So excited about this one :)

@Beanow

This comment has been minimized.

Copy link
Member Author

Beanow commented Aug 19, 2019

Added the image build step, removed the ENV export from security comments on the other PR, and use the sourcecred/* names for images.

With that I think we should be good to go :]

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Beanow added a commit to good-labs/sourcecred that referenced this pull request Aug 19, 2019
@vsoch
vsoch approved these changes Aug 19, 2019
Copy link
Contributor

vsoch left a comment

Looks good to me! Along with after merge of the other container PR, we'll want a build and deploy script here (also for CircleCI?)

@vsoch

This comment has been minimized.

Copy link
Contributor

vsoch commented Aug 23, 2019

@Beanow when we get this merged I'll open another like sourcecred/sourcecred#1320 to get an automated build and deploy.

@Beanow

This comment has been minimized.

Copy link
Member Author

Beanow commented Aug 23, 2019

Currently the readme example relies on sourcecred/sourcecred being present. So I think it would be nice to set things up on dockerhub to manually deploy v0.4.0. I've held off on a merge for that.

@Beanow

This comment has been minimized.

Copy link
Member Author

Beanow commented Aug 27, 2019

Previous comment is resolved now that there's a manual build for sourcecred/sourcecred.
See sourcecred/sourcecred#1331 / https://hub.docker.com/r/sourcecred/sourcecred/tags

This can be merged as well.

@Beanow Beanow merged commit ce953c9 into master Aug 27, 2019
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@Beanow Beanow deleted the pr/dockerfile branch Aug 27, 2019
@vsoch vsoch mentioned this pull request Sep 19, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can鈥檛 perform that action at this time.