dockerize fx-crash-sig #20

willkg · 2020-08-27T16:42:47Z

This adds some scaffolding to make it easier to maintain fx-crash-sig and also to generate a docker image and use it for development things.

What works:

make build
setting USER_ID and GROUP_ID in .env so that make build builds the docker image using specified uid/gid easing file permission issues on Linux
make test
CI

What doesn't work:

make lint and make reformat don't work--that'll get covered in another PR since it involves a lot of reformatting changes
make lint isn't run in CI
all the GCR stuff

Fixes #19.

* add Makefile for easier maintenance * add Dockerfile for building a docker image * tweak how requirements are required and add dev requirements * move tests to their own directory outside of the fx_crash_sig module * update .gitignore accordingly

Note that this doesn't run linting in CI--going to do that in a future PR.

This fixes input handling in fx-crash-sig command so that if there's nothing in stdin to slurp, it raises an error rather than hangs. This also changes fx-crash-sig command to return an error exit code when it doesn't work.

willkg · 2020-08-27T16:44:17Z

.circleci/config.yml

 orbs:
-  python: circleci/python@0.2.1
+  gcp-gcr: circleci/gcp-gcr@0.7.1


I redid the circle configuration to use what play-store-export is using which follows the guidelines in Building and Deploying Containers to GCR.

It's likely it's broken--I'll hone it in this PR.

Also, I commented out sections we'll want to implement in future PRs.

Dockerfile

willkg · 2020-08-27T16:47:26Z

Makefile

+help:
+	$(info Available rules:)
+	$(info )
+	@fgrep -h "##" Makefile | fgrep -v fgrep | sed 's/\(.*\):.*##/\1:/' | column -t -s '|'


This looks gross, but it makes this more user-friendly. It allows you to do make or make help and get help for the targets in the makefile.

willkg · 2020-08-27T16:49:29Z

Makefile

+	then \
+	echo "Creating .env file..."; \
+	echo "# USER_ID=\n# GROUP_ID=\n" > .env; \
+	fi


When running black in the container on macOS, everything is fine because docker on macOS is "fun" about what user it's running as and permissions to the file system.

On Linux, this fails because the container has a uid/gid of 10001/10001 and that's not the same as my uid/gid, so then when I run black, I just get a lot of permission issues. So I do this thing where it allows for a .env override of what uid/gid to use to build the user in the container so then I don't get permission issues, but it does it in a way that doesn't affect anyone else and works fine on macOS.

Nice! I knew about that problem but didn't know about that solution

It's the least painful one I've found so far. I'd love to know if there are better ones.

I tossed around tweaking it to auto-detect whether the host is macos or linux and set these accordingly. I'll tinker with that some day.

willkg · 2020-08-27T16:50:17Z

Makefile

+.PHONY: build
+build: .env  ## | Build docker image
+	docker-compose build --build-arg USER_ID=${USER_ID} --build-arg GROUP_ID=${GROUP_ID} app
+	touch .docker-build


The .docker-build is a real file, so then it'll get handled by make and targets that depend on it will build the image if it needs building.

willkg · 2020-08-27T16:51:54Z

fx_crash_sig/cmd_get_crash_sig.py

+    if sys.stdin.isatty():
+        print('fx-crash-sig: Failed: pass crash trace using stdin')
+        sys.exit(1)
+


This checks to see if something is being piped in via stdin and if not, prints and error and exits. This prevents the case where someone runs fx-crash-sig and it just hangs which isn't very informative.

If I had my druthers, I'd redo this command to use click and make it more user-friendly. But I'm not sure how much value we get out of that since it's not clear who's using this as installed from PyPI.

Makes sense. Personally I think we should just stop uploading to pypi until we have a clear use case for it.

👍 Less is more.

willkg · 2020-08-27T16:52:28Z

fx_crash_sig/tests.py

@@ -1,43 +0,0 @@
-# This Source Code Form is subject to the terms of the Mozilla Public


I moved this to tests/test_symbolicator.py, but that's not obvious from the PR changes.

willkg · 2020-08-27T16:53:10Z

requirements.txt

-requests==2.22.0
-siggen<2
-ujson==1.35
-google-cloud-bigquery==1.27.2


Because fx-crash-sig is both a library on PyPI and also (now) an etl job, I moved the requirements to be in setup.py which gets run in both scenarios.

setup.cfg

willkg · 2020-08-27T17:49:30Z

So, I broke CircleCI for reasons I'm unclear on, but I can't get it to run again with this PR because it's all mad. I don't want to close this PR with its comments and start a new PR, so I think I'll fix the Circle CI configuration and whatever else it discovers is broken in a future PR.

willkg · 2020-08-27T19:31:03Z

Also, I messed up and the branch this is against is in my fork. I'll do that right next time around.

wlach

Feel free to ping me when you're ready for a full review!

willkg · 2020-08-31T20:45:33Z

Yay! I got circle to build the docker image and run the tests in it. I'll do linting and reformatting in another PR.

@wlach ^^^ That's ready for a final review.

wlach

Looks good!

wlach · 2020-09-02T14:24:38Z

Dockerfile

+
+RUN apt-get update && \
+    apt-get install -y build-essential 
+


Borrowing from https://github.com/mozilla/missioncontrol-v2/blob/master/Dockerfile:

Suggested change

# clean up apt-get cruft

RUN apt-get autoremove -y && apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

I'll add that. It knocks 18mb off the size of the image.

willkg added 6 commits August 27, 2020 12:27

Initial pass at dockerization (#19)

50a7354

* add Makefile for easier maintenance * add Dockerfile for building a docker image * tweak how requirements are required and add dev requirements * move tests to their own directory outside of the fx_crash_sig module * update .gitignore accordingly

Rework tests to work in pytest (#19)

01edf80

Add .cache to .gitignore (#19)

9b349cb

Update README (#19)

683a04c

Update CI to build docker image and test with it (#19)

6fd25f0

Note that this doesn't run linting in CI--going to do that in a future PR.

Fix input handling and add exit codes

c30e3c4

This fixes input handling in fx-crash-sig command so that if there's nothing in stdin to slurp, it raises an error rather than hangs. This also changes fx-crash-sig command to return an error exit code when it doesn't work.

willkg requested a review from wlach August 27, 2020 16:42

willkg commented Aug 27, 2020

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

willkg commented Aug 27, 2020

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

willkg added 2 commits August 27, 2020 12:53

Fix inadvertently commented out line

1e6547d

Add versioning to config.yml file

9721e0b

wlach reviewed Aug 31, 2020

View reviewed changes

willkg added 2 commits August 31, 2020 15:58

Nix maintainer from Dockerfile

2a529ad

Rework lint and test so they work in Makefile and CI

e52176b

wlach self-requested a review September 2, 2020 14:22

wlach approved these changes Sep 2, 2020

View reviewed changes

Add apt cleanup to reduce image size

5d0411c

willkg merged commit 2b9e625 into mozilla-services:master Sep 2, 2020

willkg deleted the dockerize branch September 2, 2020 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dockerize fx-crash-sig #20

dockerize fx-crash-sig #20

willkg commented Aug 27, 2020 •

edited

Loading

willkg Aug 27, 2020

willkg Aug 27, 2020

willkg Aug 27, 2020

wlach Aug 31, 2020

willkg Aug 31, 2020

willkg Aug 27, 2020

willkg Aug 27, 2020

wlach Aug 31, 2020

willkg Aug 31, 2020

willkg Aug 27, 2020

willkg Aug 27, 2020

willkg commented Aug 27, 2020 •

edited

Loading

willkg commented Aug 27, 2020

wlach left a comment

willkg commented Aug 31, 2020

wlach left a comment

wlach Sep 2, 2020

willkg Sep 2, 2020

		@@ -1,43 +0,0 @@
		# This Source Code Form is subject to the terms of the Mozilla Public


	# clean up apt-get cruft
	RUN apt-get autoremove -y && apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

dockerize fx-crash-sig #20

dockerize fx-crash-sig #20

Conversation

willkg commented Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willkg commented Aug 27, 2020 • edited Loading

willkg commented Aug 27, 2020

wlach left a comment

Choose a reason for hiding this comment

willkg commented Aug 31, 2020

wlach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willkg commented Aug 27, 2020 •

edited

Loading

willkg commented Aug 27, 2020 •

edited

Loading