Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Docker dev container #338

Closed
wants to merge 3 commits into from

Conversation

daniel-shuy
Copy link

@daniel-shuy daniel-shuy commented Oct 31, 2022

TL;DR

Add Docker dev container to have consistent, reproducible dev environments

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

The goal is to run make targets in a Docker container to have a consistent, reproducible dev environment (same version of Golang and tooling).

Initially I was thinking of using the approach detailed in https://vbehar.medium.com/makefile-tip-of-the-day-run-with-or-without-docker-9c00ad84a700, e.g.

# Makefile

ifneq ($(SKIP_DOCKER),true)
	PROJECT_ROOT := $(dir $(realpath $(firstword $(MAKEFILE_LIST))))

	DOCKER_IMG := golang:1.18.5
	DOCKER_CMD := docker run \
		-it \
		--rm \
		-e SKIP_DOCKER=true \
		-e REPOSITORY=$(REPOSITORY) \
		-v $(PROJECT_ROOT):/usr/src/flyteidl \
		-w /usr/src/flyteidl \
		$(DOCKER_IMG)
endif

Then it can be added before any command to run it in a Docker container, or locally if SKIP_DOCKER is set to true, e.g.

.PHONY: generate
generate: update_boilerplate install doc_gen_deps # get latest boiler plate, install tools, generate protos, mock, pflags and  get doc dependencies
	./generate_protos.sh
	$(DOCKER_CMD) ./generate_mocks.sh
	$(DOCKER_CMD) go generate ./...

Unfortunately this doesn't work because generate depends on install, which is from boilerplate/flyte/golang_test_targets/Makefile. To use this approach will require modifying boilerplate/flyte/golang_test_targets/Makefile to run in the container as well, but boilerplate/flyte/golang_test_targets/Makefile is from another repository (https://github.com/flyteorg/boilerplate).

Therefore I went with my current approach, using a Dockerfile to build a Docker image with tools and dependencies installed (using make install), and using a Docker Compose file to map the volume.

And while I was at it, I also decided to add support for VS Code's Dev Containers (https://code.visualstudio.com/docs/devcontainers/create-dev-container#_extend-your-docker-compose-file-for-development).

Tracking Issue

fixes flyteorg/flyte#2911

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Oct 31, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@@ -0,0 +1,11 @@
FROM golang:1.18.5
Copy link
Author

Choose a reason for hiding this comment

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

Is 1.18.5 the correct Golang version?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is ok. Most people are using a flavor of 1.18 anyways.

We're also investing in a monorepo, which will make this kind of standardization a lot easier.

@codecov
Copy link

codecov bot commented Oct 31, 2022

Codecov Report

Merging #338 (8530916) into master (446b575) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #338   +/-   ##
=======================================
  Coverage   75.46%   75.46%           
=======================================
  Files          18       18           
  Lines        1174     1174           
=======================================
  Hits          886      886           
  Misses        237      237           
  Partials       51       51           
Flag Coverage Δ
unittests 75.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hamersaw
Copy link
Contributor

hamersaw commented Nov 4, 2022

@daniel-shuy I tried to run this locally and I think there may be a bit more tweaking to do. So after starting the compose container and entering it, the call to make generate is failing because the generate-protos.sh script requires docker (which is not in our dev container). Of course this is still useful for calling generate-mocks.sh and go generate ./..., but I'm wondering if modifying the Makefile is the correct way to clean this up in the near-term.

@daniel-shuy
Copy link
Author

@hamersaw ah, right. Maybe we should use docker:dind as the base image and install golang in it instead

@hamersaw
Copy link
Contributor

Going to punt on this until we get the monorepo setup and we can create a singular build setup for all Flyte components.

@hamersaw hamersaw closed this Nov 23, 2022
@daniel-shuy daniel-shuy deleted the docker-dev-container branch June 29, 2023 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Housekeeping] Dockerize call to "go generate" in flyteidl makefile
3 participants