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

[WIP] Add symlink parameter #468

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

Conversation

Leo6Leo
Copy link

@Leo6Leo Leo6Leo commented Nov 20, 2024

fixes #441

Summary of the change I have made:
Added an option to follow symbolic links when loading the registry in various parts of the codebase. The primary change is the addition of a follow_symlinks parameter, which is passed through multiple functions and structures to control this behavior.

Key changes include:

  • Added follow_symlinks param as WeaverArg across SchemaResolver, SnippetGenerator, and ResolvedSemconvRegistry to enable optional symbolic link following during registry operations.
    -The WeaverArgs struct impacted various commands (check, generate, resolve, update-markdown, stats) to include and handle the follow_symlinks option.
  • Modified SchemaResolver to configure walkdir::WalkDir with the follow_symlinks parameter and use the follow_links function.
  • Modified the existing tests so that they will default to use follow_symlinks = false.

The ideal outcome would be:
After running the command

weaver registry generate -r ./semantic-conventions/model -t ./semantic-conventions/templates markdown -f

weaver will read symbolic linked folders under sementic-conventions/model and generate the artifacts.

PS: I try to raise this PR first to get some early feedback. As I am new to Rust and Otel, I am open to suggestions and feedback.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

This is going in the right direction.

src/main.rs Outdated
/// The WeaverArgs will be shared across all commands. So only the general options should be
/// included here.
#[derive(Args, Debug)]
pub struct WeaverArgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest renaming this struct to CommonRegistryArgs and moving it to src/registry/mod.rs, as it will encapsulate the common arguments for all the registry subcommands. For global arguments (e.g., --debug), Clap already provides a mechanism. However, that mechanism cannot be used here because --follow-symlinks does not apply to commands unrelated to the registry.

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.5%. Comparing base (cab3125) to head (44664c0).

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #468     +/-   ##
=======================================
- Coverage   72.5%   72.5%   -0.1%     
=======================================
  Files         49      49             
  Lines       3635    3634      -1     
=======================================
- Hits        2639    2637      -2     
- Misses       996     997      +1     

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


🚨 Try these New Features:

@Leo6Leo
Copy link
Author

Leo6Leo commented Nov 21, 2024

I have fixed the changes you mentioned above and I manually tested the command with a symbolic link folder, and it worked flawlessly. Please let me know if there are any other major changes needed; otherwise, I’ll proceed with writing the tests! @lquerel

@Leo6Leo Leo6Leo requested a review from lquerel November 21, 2024 06:04
@lquerel
Copy link
Contributor

lquerel commented Nov 22, 2024

I have fixed the changes you mentioned above and I manually tested the command with a symbolic link folder, and it worked flawlessly. Please let me know if there are any other major changes needed; otherwise, I’ll proceed with writing the tests! @lquerel

@Leo6Leo Sounds good. Let me know when you are ready for the next review. Thanks

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.

Generate attributes from files in symlinked directories
2 participants