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 Dockerfile and docker build action #192

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

Jasper-Ben
Copy link
Contributor

Add Dockerfile. Also, build, tag and push docker image on push

@Jasper-Ben
Copy link
Contributor Author

Jasper-Ben commented Aug 30, 2022

It would be great to have todocheck as prebuilt container. e.g. this would make integration in CI systems easier, as running jobs as container has become the defacto standard.

@Jasper-Ben
Copy link
Contributor Author

Also relates to #83

@Jasper-Ben
Copy link
Contributor Author

Jasper-Ben commented Sep 5, 2022

👋 @preslavmihaylov just curious if you would have the time to review this any time soon (no pressure though 😅)? Would greatly improve things on our end 🙂. If you need any additional information from me, please don't hesitate to ask.

@preslavmihaylov
Copy link
Owner

preslavmihaylov commented Sep 11, 2022

@Jasper-Ben I'm not sure this is going to work.

When I copied your Dockerfile locally, I did:

docker build . -t todocheck:test

Afterwards, I created a folder with main.go:

package main

import "fmt"

// TODO: invalid
func main() {
	fmt.Println("hello world")
}

And a .todocheck.yaml config:

origin: github.com/user/repository
issue_tracker: GITHUB

After I run:

docker run -it todocheck:test

I get the following error:

2022/09/11 07:01:10 couldn't open configuration file: file ./.todocheck.yaml not found: unable to automatically detect issue tracker: open ./.git/config: no such file or directory

I believe this is because todocheck is running in the context of its container, rather than my local file system.
Could you explain how this is supposed to work in CI or when used locally or if I'm doing anything incorrectly?

@Jasper-Ben
Copy link
Contributor Author

Jasper-Ben commented Sep 11, 2022

Hi!
Yes, you guessed the reason correctly. I should include more details in the README file. The most straight forward way for mounting the project's directory into the container. Additionally, you might have to parse the tracker auth token into the container. When running it locally, this could look like this:

docker run -it -v /path/to/project/on/host:/project -e TODOCHECK_AUTH_TOKEN=<token>  todocheck:test --basepath /project

I will update the README.

@Jasper-Ben
Copy link
Contributor Author

Also another thing I noticed is that you add some versioning during the build of todocheck: https://github.com/preslavmihaylov/todocheck/blob/master/Makefile#L5

This currently is not the case with the docker build. I don't think it is a huge deal, since the docker tag itself can be used to identify the version, but if you say that needs to be amended, I could take a look into it.

@preslavmihaylov
Copy link
Owner

I think a more sensible approach would be to configure the docker image build to be done on new releases & it should match the version of the release.

@Jasper-Ben
Copy link
Contributor Author

I think a more sensible approach would be to configure the docker image build to be done on new releases & it should match the version of the release.

This (and more) is basically what the GitHub action will do. If you tag and push a git commit, the GitHub action will automatically build and tag the docker image appropriately. Optionally, but from experience recommended (minimum: at least for the master branch), It also builds and pushes an appropriately tagged docker image for a push to any branch, keeping the latest (head of branch). This is useful if a certain feature is required but hasn't been merged or released yet. The user of the software therefore could opt to use the image for the latest master branch commit until the next release is available.

@preslavmihaylov
Copy link
Owner

I'll make a todocheck fork & play around with this to validate that it works correctly

@preslavmihaylov
Copy link
Owner

Hey @Jasper-Ben I managed to play around with this in a fork & set it up nicely.

Check out #200

Via those changes:

  • docker image is built on push to master only + new releases
  • Running todocheck --version now corresponds to the git sha (if built via master) or the version (if via release)

Would you like to replicate those changes in this PR so that I can merge it? I could just merge that one, but I don't want to steal the credits :)

Adds support for container builds. Github action will automatically
build images on master branches and on releases and push them to the
github container registry.
@Jasper-Ben
Copy link
Contributor Author

Jasper-Ben commented Sep 18, 2022

Would you like to replicate those changes in this PR so that I can merge it?

Done :)

I've done some further cleanup to the Dockerfile (removed unnecessary cp and the now unused make package), squashed and pushed. Please verify :)

Copy link
Owner

@preslavmihaylov preslavmihaylov left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@preslavmihaylov preslavmihaylov merged commit 845d11f into preslavmihaylov:master Sep 19, 2022
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.

2 participants