Skip to content

Conversation

@Almaju
Copy link
Contributor

@Almaju Almaju commented Sep 19, 2025

Refactored the ads-client to use the existing context_id crate for uuid generation and lifecycle.

Since we are changing existing methods, the tests remain largely the same.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@luc-lisi
Copy link
Contributor

@mikeconley

I wanted to reach out because I believe you are the owner (or at least main contributor) for this ContextId component. I notice in the repo, the README says

It is currently under construction and not yet used.

This ads-client component is also still unused so no need for this to be production ready or anything yet, but I wanted to make sure that it was okay to start integrating with this component or if there are anticipated major changes coming with this ContextId component.

Thanks!

@Almaju Almaju force-pushed the MAJC-184-new-hire-integrate-context-id-into-ads-client branch from cfe4b1f to e2d9fb4 Compare September 22, 2025 20:58
@luc-lisi
Copy link
Contributor

Confirmed with @mikeconley the context_id component is good for us to use. All LGTM.

luc-lisi
luc-lisi previously approved these changes Sep 25, 2025
Copy link
Contributor

@luc-lisi luc-lisi left a comment

Choose a reason for hiding this comment

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

Awesome job! Looks all great to me 🚢

@Almaju Almaju force-pushed the MAJC-184-new-hire-integrate-context-id-into-ads-client branch from 42d2532 to 14c332b Compare September 25, 2025 18:33
@mergify mergify bot dismissed luc-lisi’s stale review September 25, 2025 18:34

The pull request has been modified, dismissing previous reviews.

luc-lisi
luc-lisi previously approved these changes Sep 25, 2025
context_id,
context_id_component: ContextIDComponent::new(
&context_id,
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should eventually figure out how we want to handle time here, but approving as this is something we can look into more later and don't want to block this merge.

@mergify mergify bot dismissed luc-lisi’s stale review September 25, 2025 20:36

The pull request has been modified, dismissing previous reviews.

@Almaju Almaju force-pushed the MAJC-184-new-hire-integrate-context-id-into-ads-client branch 7 times, most recently from 8e179ae to 8221538 Compare September 26, 2025 21:16
@Almaju Almaju requested a review from luc-lisi September 26, 2025 21:56
luc-lisi
luc-lisi previously approved these changes Sep 26, 2025
@Almaju Almaju force-pushed the MAJC-184-new-hire-integrate-context-id-into-ads-client branch from 8221538 to 52c390c Compare September 26, 2025 22:26
@mergify mergify bot dismissed luc-lisi’s stale review September 26, 2025 22:26

The pull request has been modified, dismissing previous reviews.

@luc-lisi luc-lisi added this pull request to the merge queue Sep 26, 2025
Merged via the queue into mozilla:main with commit 569dc9a Sep 26, 2025
14 checks passed
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.

2 participants