Skip to content

Conversation

@khushijain21
Copy link
Contributor

@khushijain21 khushijain21 commented Aug 29, 2025

What does this PR do?

This PR supports additonal options on certain method intlscommon package to accept a local logger.
Note: This PR includes breaking changes as well - but those methods are not used by elastic-agent ensuring the blast radius is small

This is part of the effort to get rid of global states.

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Related issues

@khushijain21 khushijain21 requested a review from a team as a code owner August 29, 2025 06:45
@khushijain21 khushijain21 requested review from andrzej-stencel and leehinman and removed request for a team August 29, 2025 06:45
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

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

I agree a phased approach to the breaking change may help users of this API. Other than that, the PR looks good. I'm approving it from my side and will let @kruskall and @khushijain21 reach a conclusion on the breaking change approach.

@khushijain21
Copy link
Contributor Author

As discussed in our sync meeting - we would rather do a breaking change now that would cause some churn than have users of this API continue to use global logger.

@khushijain21 khushijain21 enabled auto-merge (squash) September 5, 2025 04:44
@kruskall
Copy link
Member

kruskall commented Sep 5, 2025

As discussed in our sync meeting - we would rather do a breaking change now that would cause some churn than have users of this API continue to use global logger.

the suggestion was to remove the global logger in a backward compatible way. The current method should be updated to use noop logger if nothing is passed.
I've added a comment to #348 (comment)

I'll dismiss my review to unblock but don't feel comfortable approving.

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

Labels

Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants