Skip to content

feat: configure json logger for the whole gitingest module #451

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

feat: configure json logger for the whole gitingest module #451

wants to merge 5 commits into from

Conversation

@ix-56h
Copy link
Contributor

@ix-56h ix-56h commented Jul 25, 2025 •
edited
Loading

Closes #357

Topic

Current gitingest package doesn't use proper logging, this PR should add a json format logging system.

Todo

  • Package: Add logger for gitingest module
  • Backend: Add logger for backend app

@ix-56h ix-56h changed the title Configure json logger for the whole gitingest module Feat: Configure json logger for the whole gitingest module Jul 25, 2025
@ix-56h ix-56h marked this pull request as draft July 25, 2025 15:34
@MickaelCa MickaelCa changed the title Feat: Configure json logger for the whole gitingest module feat: configure json logger for the whole gitingest module Jul 25, 2025
NicolasIRAGNE
NicolasIRAGNE requested changes Jul 27, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented Jul 27, 2025

This pull request has merge conflicts that must be resolved before it can be merged.

@ix-56h ix-56h force-pushed the feat/json-logger branch from 27e4973 to b95fc8a Compare July 28, 2025 14:24
@ix-56h ix-56h requested a review from NicolasIRAGNE July 28, 2025 14:29
@github-actions
Copy link

github-actions bot commented Jul 28, 2025

This pull request has resolved merge conflicts and is ready for review.

@github-actions
Copy link

github-actions bot commented Jul 29, 2025

This pull request has merge conflicts that must be resolved before it can be merged.

@MickaelCa MickaelCa mentioned this pull request Jul 29, 2025
Merged
@ix-56h ix-56h marked this pull request as ready for review July 30, 2025 13:36
@ix-56h
Copy link
Contributor Author

ix-56h commented Jul 30, 2025 •
edited
Loading

@NicolasIRAGNE I had to make few changes to :

  • src/server/query_processor.py:generate_s3_file_path()
  • and src/server/s3_utils.py:generate_s3_file_path

When fixing the conflicts, Pylint wasn't very happy because we were passing query.url : str | None to generate_s3_file_path(str, str...), so I fixed that and made the function more explicit with the arguments (passing the IngestionQuery object directly).

@github-actions
Copy link

github-actions bot commented Jul 30, 2025

This pull request has resolved merge conflicts and is ready for review.

@coderamp-labs coderamp-labs locked and limited conversation to collaborators Jul 30, 2025
@ix-56h ix-56h closed this Jul 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@NicolasIRAGNE NicolasIRAGNE Awaiting requested review from NicolasIRAGNE

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: modern logging system

3 participants

@ix-56h @NicolasIRAGNE