Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Rename Result results_id JSON field name to deal_id #469

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

bgins
Copy link
Contributor

@bgins bgins commented Dec 11, 2024

Summary

This pull request makes the following changes:

  • Change the JSON name for Result.DealID from results_id to deal_id
  • Add custom marshal and unmarshal Result functions for backwards compatibility
  • Add tests to verify marshal and unmarshal functions

The Result struct on main uses the JSON name results_id for its DealID. This pull request changes the JSON name to deal_id.

Test plan

We have added property-based tests that check round trip serialization/deserialization and JSON backwards compatibility. Run the test suite and check that the new tests pass or run the tests directly:

go test pkg/data/data_test.go pkg/data/data.go -v --tags="unit" --count 1

Running a test job takes a few more steps than usual:

  • Start the base services
  • Start the solver on this pull request's branch (bgins/fix-rename-results-id-field)
  • Start a resource provider from main or using a binary at 2.10.0
  • CLI run a job on main or using a binary at 2.10.0

These steps check compatibility with a resource provider and job creator that use results_id.

Details

Resource providers will continue to create results with the results_id field name until they have upgraded, and job creators will expect a results_id field until they have upgraded.

We have implemented a Result compatibility layer with custom serialization and deserialization:

  • MarshalJSON adds the results_id field for clients that expect it
  • UnmarshalJSON checks for a results_id field from older clients and assigns it to DealID

We can remove this compatibility layer once clients have upgraded to a version that uses deal_id.

@bgins bgins self-assigned this Dec 11, 2024
@cla-bot cla-bot bot added the cla-signed label Dec 11, 2024
@github-actions github-actions bot added the fix label Dec 11, 2024
@bgins bgins marked this pull request as ready for review December 12, 2024 00:00
@bgins bgins requested a review from a team as a code owner December 12, 2024 00:00
@bgins bgins changed the title fix: Rename results_id JSON field name to deal_id fix: Rename Result results_id JSON field name to deal_id Dec 12, 2024
Copy link
Collaborator

@walkah walkah left a comment

Choose a reason for hiding this comment

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

Tests pass locally - and I was able to run a job from main.

Excellent test plan as usual. Thanks, @bgins !

@bgins bgins merged commit 0050f2c into main Dec 17, 2024
11 checks passed
@bgins bgins deleted the bgins/fix-rename-results-id-field branch December 17, 2024 20:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants