Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 11, 2024

What do these changes do?

In PR #6701, the test test_EC2_INSTANCES_ALLOWED_TYPES_empty_not_allowed fails with the new Pydantic version, but it unexpectedly passes with the previous version. This PR modifies the test to highlight a bug in our BaseCustomsSettings logic when using Pydantic v1.

The bug originates from a workaround initially applied by @YuryHrytsuk on the refactoring of docker-compose environments (this is also related to this change ITISFoundation/osparc-ops-environments#866), which involved using an empty dictionary {} as an environment variable value to simulate an "unset" state. This workaround impacts fields set with auto-default-factory=True, where empty dicts {} mark fields as UNSET.

In particular, this test case focuses on the field ApplicationSettings.AUTOSCALING_EC2_INSTANCES: EC2InstancesSettings | None, which is a nullable field with an auto-default factory. The test_EC2_INSTANCES_ALLOWED_TYPES_empty_not_allowed test expects the auto-default factory to return None. Instead, it raises a ValidationError because AUTOSCALING_EC2_INSTANCES={} is misinterpreted as a valid assignment instead of as an unset state.

This PR enhances test_EC2_INSTANCES_ALLOWED_TYPES_empty_not_allowed to establish a foundation for the migration to Pydantic v2, as outlined in PR #6701. In this migration, we will address the issue by enforcing that AUTOSCALING_EC2_INSTANCES={} is treated as an explicit assignment rather than an "unset" sentinel.

Related issue/s

How to test

cd services/autoscaling
make install-dev
pytest -vv tests/unit -k test_EC2_INSTANCES_ALLOWED_TYPES_empty_not_allowed

Dev-ops

@pcrespov pcrespov self-assigned this Nov 11, 2024
@pcrespov pcrespov added this to the Event Horizon milestone Nov 11, 2024
@pcrespov pcrespov marked this pull request as ready for review November 11, 2024 15:49
@pcrespov pcrespov requested a review from sanderegg as a code owner November 11, 2024 15:49
@pcrespov pcrespov added t:maintenance Some planned maintenance work a:autoscaling autoscaling service in simcore's stack labels Nov 11, 2024
@codecov
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.85%. Comparing base (ec1e84e) to head (6b4d346).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ec1e84e) and HEAD (6b4d346). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (ec1e84e) HEAD (6b4d346)
unittests 31 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6705       +/-   ##
===========================================
- Coverage   87.75%   66.85%   -20.91%     
===========================================
  Files        1569      651      -918     
  Lines       63153    33131    -30022     
  Branches     2116      265     -1851     
===========================================
- Hits        55423    22150    -33273     
- Misses       7405    10920     +3515     
+ Partials      325       61      -264     
Flag Coverage Δ
integrationtests 64.90% <ø> (+0.02%) ⬆️
unittests 87.30% <ø> (+1.57%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.44% <ø> (-7.84%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling 95.21% <ø> (ø)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 76.40% <ø> (-14.54%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 59.79% <ø> (-30.01%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
osparc_gateway_server 79.42% <ø> (-5.73%) ⬇️
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 59.87% <ø> (-28.85%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec1e84e...6b4d346. Read the comment docs.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

I guess there's gonna be a different PR with the fix on the pydantic-v2-migration branch, right?

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks

@pcrespov pcrespov requested a review from GitHK November 12, 2024 07:36
@pcrespov
Copy link
Member Author

pcrespov commented Nov 12, 2024

I guess there's gonna be a different PR with the fix on the pydantic-v2-migration branch, right?

yes, this is a guideline so that I can fix it in the pydantic-v2-migration branch.

@pcrespov pcrespov enabled auto-merge (squash) November 12, 2024 07:36
@sonarqubecloud
Copy link

@pcrespov pcrespov merged commit f8f67c9 into ITISFoundation:master Nov 12, 2024
84 of 89 checks passed
@pcrespov pcrespov deleted the fix/test-settings branch November 19, 2024 18:31
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jan 15, 2025
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:autoscaling autoscaling service in simcore's stack t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants