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

feat(BA-593): Configure the logging module's config file to use Pydantic #2834

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Yaminyam
Copy link
Member

@Yaminyam Yaminyam commented Sep 12, 2024

resolve #3518 (BA-593)
Ensure that the logging module's config file is validated using pydantic.

Make logging module-specific settings that wsproxy implements in its own config file common to the logging module.

If you are working on replacing trafaret with pydantic and you have resolved any dependencies on the logging module, you can replace config_pydantic.py with config.py

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version

@github-actions github-actions bot added the size:L 100~500 LoC label Sep 12, 2024
@Yaminyam Yaminyam added this to the 25.03 milestone Sep 12, 2024
@Yaminyam Yaminyam marked this pull request as ready for review September 12, 2024 07:57
@Yaminyam Yaminyam requested a review from HyeockJinKim December 9, 2024 07:05
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

  • Let's use Pydantic's alias generator to accept hyphenated field names as well.
  • Let's avoid using Annotated whenever possible to reduce verbosity.

@github-actions github-actions bot added the comp:common Related to Common component label Jan 13, 2025
@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Jan 13, 2025
@Yaminyam Yaminyam requested a review from achimnol January 14, 2025 02:06
@Yaminyam Yaminyam requested a review from HyeockJinKim January 22, 2025 08:01
@Yaminyam Yaminyam changed the title feat: Configure the logging module's config file to use Pydantic feat(BA-593): Configure the logging module's config file to use Pydantic Jan 22, 2025
Comment on lines +32 to +48
class HostPortPair(BaseSchema):
host: str = Field(examples=["127.0.0.1"])
port: int = Field(gt=0, lt=65536, examples=[8201])

def __repr__(self) -> str:
return f"{self.host}:{self.port}"

def __str__(self) -> str:
return self.__repr__()

def __getitem__(self, *args) -> int | str:
if args[0] == 0:
return self.host
elif args[0] == 1:
return self.port
else:
raise KeyError(*args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I’m wondering is why the HostPortPair class is inside the logging package.
It seems like it should be in a different package.

Copy link
Member Author

Choose a reason for hiding this comment

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

We support several logging tools and may support more in the future.
(ex, graylog, logstash)
Since these logging tools require the host address by default, we've just separated that part into a common setting.
ref.

t.Key("host"): t.String,

@HyeockJinKim
Copy link
Collaborator

Please resolve the conflict. @Yaminyam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:common Related to Common component size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to pydantic-based logging configuration schema
3 participants