Skip to content

feat: Improve dev experience by using local S3 #244

Merged
frgfm merged 36 commits intopyronear:mainfrom
Acruve15:improve-dev-experience
Oct 20, 2023
Merged

feat: Improve dev experience by using local S3 #244
frgfm merged 36 commits intopyronear:mainfrom
Acruve15:improve-dev-experience

Conversation

@Acruve15
Copy link
Copy Markdown
Contributor

Description

Linked to Issue 217. Developing locally is not possible if the developer does not have an AWS S3 access.

What was done

We used localstack to solve this issue.

update dependencies for localstack
create empty nbucket for dev environment
add localstack service
@Acruve15 Acruve15 changed the title Improve dev experience bu using local S3 Improve dev experience by using local S3 Mar 15, 2023
@frgfm frgfm self-requested a review March 19, 2023 16:05
@frgfm frgfm added this to the 0.2.0 milestone Mar 19, 2023
Copy link
Copy Markdown
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @Acruve15 🙏

I added a few comments but this is looking really good! I have one extra request: is this running in the CI tests?

Comment thread Makefile Outdated
Comment thread client/pyproject.toml Outdated
Comment thread docker-compose-dev.yml Outdated
Comment thread docker-compose-dev.yml Outdated
Comment thread docker-compose-dev.yml Outdated
Comment thread docker-compose-dev.yml Outdated
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2023

Codecov Report

Merging #244 (e6079a3) into main (5be37ab) will decrease coverage by 0.17%.
The diff coverage is n/a.

❗ Current head e6079a3 differs from pull request most recent head 98a68ab. Consider uploading reports for the commit 98a68ab to get more accurate results

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
- Coverage   94.95%   94.78%   -0.17%     
==========================================
  Files          63       57       -6     
  Lines        1505     1322     -183     
==========================================
- Hits         1429     1253     -176     
+ Misses         76       69       -7     
Flag Coverage Δ
unittests 94.78% <ø> (-0.17%) ⬇️

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

see 40 files with indirect coverage changes

Acruve15 and others added 15 commits March 27, 2023 18:33
@Acruve15
Copy link
Copy Markdown
Contributor Author

Acruve15 commented Apr 2, 2023

Thanks a lot for the PR @Acruve15 🙏

I added a few comments but this is looking really good! I have one extra request: is this running in the CI tests?

Hey @frgfm, localstack was added on part of the tests, tell me if it's good for you, thx

Copy link
Copy Markdown
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thank you very much :)

Comment thread .gitignore Outdated
Copy link
Copy Markdown
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

My bad there are still a few things to fix the CI:

  • we need to integrate that also in the python client test suite
  • strangely the test job is failing now 🤔

Could you take a look please?

@frgfm
Copy link
Copy Markdown
Member

frgfm commented May 28, 2023

Any update @Acruve15 ? 😃
It would be a shame that we don't use your work!

@blenzi
Copy link
Copy Markdown

blenzi commented Aug 2, 2023

Hi @Acruve15, very nice addition! I tried to run this locally but make test fails on the docker-compose exec localstack command. If I issue the command by hand again it works. Any idea ?

sleep to make sure localstack is up and running
better manage sleep time in CI
@Acruve15 Acruve15 changed the title Improve dev experience by using local S3 feat: Improve dev experience by using local S3 Aug 2, 2023
frgfm
frgfm previously approved these changes Oct 20, 2023
Copy link
Copy Markdown
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

The CI is failing because the PR comes from a fork and can't access repo secrets. I've just run everything locally, now everything is in place 😄

Thanks a lot @Acruve15 🙏

@frgfm frgfm merged commit 4c90d1a into pyronear:main Oct 20, 2023
@Acruve15
Copy link
Copy Markdown
Contributor Author

@frgfm thx for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants