Skip to content

Conversation

@Zaimwa9
Copy link
Contributor

@Zaimwa9 Zaimwa9 commented Nov 20, 2025

Environment.name was wrongly mapped.

Added tests to mappers

@Zaimwa9 Zaimwa9 requested a review from a team as a code owner November 20, 2025 17:03
@Zaimwa9 Zaimwa9 requested review from khvn26 and removed request for a team November 20, 2025 17:03
@Zaimwa9 Zaimwa9 requested review from emyller and removed request for khvn26 November 20, 2025 17:19
Copy link

@emyller emyller left a comment

Choose a reason for hiding this comment

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

  1. Although the fix looks solid, is it relevant to get Environment.name available in the old engine?
  2. Does this affect other behavior in the new engine? If so, we'll need to coordinate a team effort to update other SDKs. 😞

P.S. I also fixed "environment.name" in the fixture for the PHP client in order to map it correctly to the context, but didn't bother to make it useful in the old engine because seemingly it never was?

Copy link

@emyller emyller left a comment

Choose a reason for hiding this comment

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

Approving because my previous questions aren't blockers.

@Zaimwa9
Copy link
Contributor Author

Zaimwa9 commented Nov 21, 2025

The way I see it:

  1. It doesn't impact the old engine. But at least in node, the flow is:
  • Reading the document
  • Creating models (including the EnvironmentModel) => That's where I need to make it available
  • Mapper use the EnvironmentModel as a basis => That's where I need the name
    Basically, there is no "old engine" in Node, there are models that act as Data structure that have been preserved because it's still used in the sdk: make it useful in the old engine because seemingly it never was => This is spot on. It didn't exist, adding it wouldn't change the behavior of the old engine. Just an extra unused property in this old way
  1. That's the fix. If we use the project.name it means that all the evaluation based on EnvironmentName would be wrong.

@Zaimwa9 Zaimwa9 merged commit 3c1d200 into main Nov 21, 2025
4 checks passed
@emyller emyller deleted the fix/map-environment-name-and-tests branch November 21, 2025 12:42
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.

3 participants