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

fix(detections): allow azimuth = 0 #399

Closed
wants to merge 6 commits into from
Closed

Conversation

RonanMorgan
Copy link
Collaborator

@RonanMorgan RonanMorgan commented Jan 7, 2025

Currently there is an error if the azimuth is defined equal to 0

Close #395

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.83%. Comparing base (7a0dcd7) to head (60b016b).
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #399   +/-   ##
=======================================
  Coverage   84.83%   84.83%           
=======================================
  Files          35       35           
  Lines         996      996           
=======================================
  Hits          845      845           
  Misses        151      151           
Flag Coverage Δ
backend 84.35% <100.00%> (ø)
client 92.06% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MateoLostanlen
MateoLostanlen previously approved these changes Jan 7, 2025
Copy link
Member

@MateoLostanlen MateoLostanlen left a comment

Choose a reason for hiding this comment

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

Hi @RonanMorgan , thanks a lot for the fix

@frgfm frgfm changed the title Azimuth could be equal to 0 fix(detections): allow azimuth = 0 Jan 7, 2025
Copy link
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 for the PR! I added a suggestion because if we merged this modification we'll get ]-1,360[ (floats, not integers), but we want [0,360[

src/app/api/api_v1/endpoints/detections.py Outdated Show resolved Hide resolved
src/app/models.py Outdated Show resolved Hide resolved
src/app/schemas/detections.py Outdated Show resolved Hide resolved
@RonanMorgan RonanMorgan requested a review from frgfm January 9, 2025 10:48
frgfm
frgfm previously approved these changes Jan 9, 2025
Copy link
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! Only one concern: I can't figure out why the docker build job fails 🤔 (it's working on main)
Worth checking before merging

@RonanMorgan
Copy link
Collaborator Author

@frgfm I don't have any issue locally but it seems to me that the backend stay unhealthy longer than 30sec, can I change this parameters ? :

  interval: 10s
  timeout: 3s
  retries: 3

@frgfm
Copy link
Member

frgfm commented Jan 9, 2025

@RonanMorgan yes but if you do, only increase the number of retries please 🙏

@RonanMorgan
Copy link
Collaborator Author

ok I just remove the docker-compose;yml file and replaced it by docker-compose.dev;yml (which make sense now since there is a production docker-compose in ansible)

I think it was a variable error since there are default values in this one

@RonanMorgan RonanMorgan requested a review from frgfm January 10, 2025 20:53
Copy link
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.

Changing the docker env var values is quite alright and removing the somehow duplicate compose, but not too sure about removing alembic and volume binding for the DB & localstack?

This would make the docker compose solely a "test" compose, because there is no data persistence and we don't create migrations. is this a good idea? 🤔 (it fixes the CI, but my question is: won't this reduce the usability of the project?)

@RonanMorgan
Copy link
Collaborator Author

well is this used for anything else than test ? :)

I have an other docker-compose for any production usage in ansible, I think this should be enough (?)

@RonanMorgan RonanMorgan requested a review from frgfm January 13, 2025 08:31
@frgfm
Copy link
Member

frgfm commented Jan 13, 2025

I have an other docker-compose for any production usage in ansible, I think this should be enough (?)

Question of perspective, I'm open to both. Until now, we've made sure the repo had something for the dev to "try locally" what production would do, and run tests as well. You think it's not worth it?

@RonanMorgan
Copy link
Collaborator Author

RonanMorgan commented Jan 13, 2025

I think that's worth it but in this case we need to add Treaefik and test it regularly in a new vm. For me the docker-compose I removed was too far from the production one to be more useful than the .dev. one

@frgfm
Copy link
Member

frgfm commented Jan 15, 2025

Ok, let's first finish this PR topic: why in the first place did we need to change so many files to have the CI work 🤔
Especially because the CI has had no problem on main for a while. For last few PRs, we didnt have to change anything on docker or the CI config to get it to pass. If we could have that minimal change, that would awesome 👌

@frgfm
Copy link
Member

frgfm commented Jan 16, 2025

I tried my best to rebase the PR but several conflicts were a bit tideous so I created #414
Closing this one since this is now handled, thanks again!

@frgfm frgfm closed this Jan 16, 2025
@frgfm frgfm deleted the rs/fix-azimuth-0 branch January 16, 2025 15:08
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.

Azimuth can be zero
3 participants