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

poc for local + hot reloaded worker + shared #29

Closed
wants to merge 15 commits into from

Conversation

matt-codecov
Copy link
Contributor

@matt-codecov matt-codecov commented Jul 24, 2023

alrighty, i think this is ready for review

there are more ways to build worker images now:

make build # totally unchanged
make dev # new setup, hot-reloads worker
make dev-shared # new setup, hot-reloads worker and shared

the latter two tag their resulting docker images with codecov/worker-dev instead of codecov/worker so if you switch you will want to update your docker-compose.yml or whatever.

the downside of hot-reloading shared is, when you initially start the container, it takes an extra couple minutes to fully build shared. it'll look like it froze during installation, but if you come back with coffee it should be running. hot-reloading will be very speedy, it's just that initial container startup.

to toggle hot-reloading shared, just run make dev or make dev-shared and restart the codecov/worker-dev container.

to work on or update shared:

# this will reset your shared to the committed submodule hash
git submodules update --init --recursive

cd shared
git checkout <new hash>
cd ..
git add shared
git commit -m 'update shared'

this same approach should work for API as well. if it's approved here, joseph or i will do it there as well.

the submodule part

instead of installing shared from GitHub at a specified hash, make it a git submodule with that same hash checked out and install it with file:./shared#egg=shared

with just this, it's easier to work with shared. change shared, rebuild your worker image. no more pushing WIPs to GitHub to copy the hash. however, it's not enough to hot-reload

the hot-reload part

we install watchmedo which is a util that does hot-reloading. if you change a python file it'll re-run worker.sh and your changes will take effect

but shared is special. shared is imported from the site-packages directory rather than the submodule. we need another step to hot-reload shared

hot-reloading shared

we can perform an "editable" install of shared with pip install -e ./shared. this binds the installed module to the original, in-tree sources. however, if we do that while building the docker images, it'll bind the module to the copies of the sources inside the docker container.

instead, we want to run it after we've started the container. at that point, we've bind-mounted our own copy of worker/shared into the container, and when we run pip install -e ./shared, it'll bind to our copy.

for reasons unknown, this only wants to work after a non-editable install has also been performed.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #29 (11551e5) into main (aca010e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #29   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files         359      359           
  Lines       26284    26284           
=======================================
  Hits        25914    25914           
  Misses        370      370           
Flag Coverage Δ
integration 98.56% <ø> (ø)
latest-uploader-overall 98.56% <ø> (ø)
onlysomelabels 98.58% <ø> (ø)
unit 98.56% <ø> (ø)

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

Components Coverage Δ
NonTestCode 97.29% <ø> (ø)
OutsideTasks 98.34% <ø> (ø)

This change has been scanned for critical changes. Learn more

Copy link
Contributor Author

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

could you add --ignore=shared to the addopts list in worker's pytest.ini?

requirements.dev Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
dockerscripts/Dockerfile.local Outdated Show resolved Hide resolved
matt-codecov and others added 5 commits August 3, 2023 10:25
- Rename build-dev to dev
- Creation of build.base_dev target
- Requirements.dev is requirements.txt with the
  dependency on shared removed
- Use of docker build arg to know when to use
  requirements.dev

Signed-off-by: joseph-sentry <[email protected]>
@giovanni-guidini
Copy link
Contributor

One thing that is not very clear to me (possibly because I haven't really worked with git submodules), how do you specify what is the shared SHA when deploying?

Or these changes don't modify the deploy to staging/prod at all, just the local development setup?

@matt-codecov
Copy link
Contributor Author

matt-codecov commented Aug 4, 2023

in your local directory you will see shared as a directory, but git actually basically sees it as a file with a hash in it. if you checkout a commit inside the shared dir and then run git status from worker, it will show shared as having changed. if you commit that change to shared, it'll update which hash the submodule is associated with to the hash that you checked out

Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

I'll say looks good, and I'm fine with the changes. But please update README.md with the instructions that are in the PR comment 🙏

@thomasrockhu-codecov
Copy link
Contributor

@matt-codecov just curious what the status of this PR was

@matt-codecov
Copy link
Contributor Author

there was a strange CI issue, was waiting for the GHA migration to settle and just haven't gotten back to it since

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.

4 participants