feat: Added support for Harry Potter category, Sprint Name Generator Docker image, div improvements #38
Conversation
…mation on how to build Docker image locally
…tasks to build pipeline
…nd disabled some actions to not run in my repo
|
Hey @guidemetothemoon, wow, thanks for the big PR! 😮 |
|
Thanks a lot for the PR @guidemetothemoon! I'll try to review it as soon as possible :) |
|
Sorry for the delay - busy week. I have a long train ride coming up today and will use it to review it :) |
build/package/Dockerfile
Outdated
| RUN go install github.com/shurcooL/goexec@latest | ||
| RUN go get github.com/shurcooL/go-goon | ||
|
|
||
| EXPOSE 80 | ||
|
|
||
| CMD ["goexec", "http.ListenAndServe(`:80`, http.FileServer(http.Dir(`.`)))"] |
There was a problem hiding this comment.
What's the reason for adding goexec and go-goon?
go-goon is deprecated afaik.
Why can't we just have the mux http APIs as part of a regular go executable?
Or is this for starting a web server to serve the web site locally?
If it's for local development I'd say we should try and keep both the APIs and the local development setup.
I think for the local development using nginx as the container image might be easiest
There was a problem hiding this comment.
@flostadler thanks for the review! I'm pretty new at Go development so in this case I've followed one of the recommended approaches on containerizing Go+WASM application. Guidelines might not have been 100% up-to-date though since you mentioned that go-goon is deprecated.
Now, I'm not quite sure why you need to build a container to only have APIs? I thought that the purpose of introducing a container in this case is to be able to run and host this application wherever you want instead of using publicly available version for instance... Therefore I updated Dockerfile with a thought to make it possible to run Sprint Name Generator application as a whole, in a container. I tried to make the original container work but I didn't manage it, maybe due to the fact that I didn't have the whole picture on why you would want to only make the APIs available in a container and which APIs would that be?
I can update the Dockerfile by removing the outdated components so that an application could be run as a container but should the API version also be available you think?
Also, please check PR description - I think that you will need to make a few adjustments to make the build work once all changes in this PR are OK.
There was a problem hiding this comment.
@flostadler , today I digged more into how Golang and WASM hang together and of course it's perfectly simple and straightforward to use NGINX as a web server in this case! Thanks a lot for challenging me on this, learned a lot of valuable stuff today! 😸 I've now tested and updated the Dockerfile to support running Sprint Name Generator in a container. Please let me know if anything else needs to be updated.
|
@flostadler awesome, I'm glad to help out! Just a gentle reminder in case you missed it in my previous comment: you will need to add Docker username and access token to GitHub repo secrets and fix the access token to deploy to GitHub Pages so that build goes through successfully (it fails now). More info is added to the PR description. |
|
Yeah I already took note about that and will do it once I'm done with work for today :) If I have some questions I'll add them here, thanks! |
Hello @flostadler @wederer !☺️
I have added a few enhancements to Sprint Name Generator, I hope you don't mind
I would really appreciate a review. You can also check my forked repo and respective repo on Docker Hub with latest working Sprint Name Generator container image: https://github.com/guidemetothemoon/sprint-name-generator
Following changes have been made:
The final result will look like this:
Important note: a few updates are needed from your side as repo owners to get it working.
DOCKER_HUB_USERNAME-> username of the one owning the Sprint Name Generator repo on Docker Hub, I assume it should beflostadler?DOCKER_HUB_ACCESS_TOKEN-> access token that gives permissions for the build pipeline to access Sprint Name Generator repo on Docker Hub (to push and pull images).Linked issues:
#8
#6