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

FF-2465 adds "environment" and "allocation.name" to details #107

Merged

Conversation

greghuels
Copy link
Contributor

@greghuels greghuels commented Jun 28, 2024

FF-2465

Motivation and Context

This change was motivated by internal feedback.

Description

Adds environment and assignments[].name to the new "assignment details" functionality.

How has this been tested?

verified test data works with updated change

Manual testing:
Screenshot 2024-07-01 at 12 05 21 PM

Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Love this quick iteration on demo day feedback 💪

Copy link
Contributor

@sameerank sameerank left a comment

Choose a reason for hiding this comment

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

🙌

@greghuels greghuels force-pushed the greg/FF-2526/ufc-environment branch 2 times, most recently from b396353 to b8f67b8 Compare July 1, 2024 14:11
@greghuels
Copy link
Contributor Author

I verified locally that the test data passes with this change.

Comment on lines +252 to +254
describe.each(getTestFilePaths())('for file: %s', (testFilePath: string) => {
const testCase = parseJSON(testFilePath);
describe.each(testCase.subjects.map(({ subjectKey }) => subjectKey))(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-wrote how this works so that the error messages are more clear and so that it's easier to debug

Copy link
Contributor

@sameerank sameerank left a comment

Choose a reason for hiding this comment

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

👍 Consider making the allocation names and keys more closely resemble how they would look in the config generated by the server

const flagEvaluationDetailsBuilder = new FlagEvaluationDetailsBuilder(
configDetails.configEnvironment.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: typescript ensures we don't need optional chaining here because of the environment interface in ConfigDetails, configEnvironment: Environment;

@greghuels greghuels force-pushed the greg/FF-2526/ufc-environment branch from 091be90 to b57be38 Compare July 1, 2024 19:07
@greghuels greghuels merged commit a7dfb70 into greg/FF-2465/evaluation-reasons Jul 1, 2024
2 checks passed
@greghuels greghuels deleted the greg/FF-2526/ufc-environment branch July 1, 2024 19:09
greghuels added a commit that referenced this pull request Jul 2, 2024
* FF-2465 adds "environment" and "allocation.name" to details

* FF-2526 update to accommodate EnvironmentDto

* FF-2526 FF-2526 fix "environment" and code review changes
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