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

♻️ 📝 Minor refactor and doc of autoscaling service #6551

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 16, 2024

What do these changes do?

Minor refactor and documentation update for autoscaling service

  • ♻️ Refactored get_ec2_instance_capabilities to guarantee a sorted return value.
  • 🎨 Added a warning log when EC2_INSTANCES_ALLOWED_TYPE is empty.
    • Note: While I believe we should prevent this scenario altogether, I opted to maintain the current logic and only introduce a warning to avoid altering the original intent.
  • 📝♻️ Added minimal doc or refactor code to clarify code intentions.

Related issue/s

How to test

cd services/autoscaling
make install-dev
pytest -vv tests/unit/test_modules_auto_scaling_core.py

Dev-ops checklist

None

@pcrespov pcrespov self-assigned this Oct 16, 2024
@pcrespov pcrespov added a:autoscaling autoscaling service in simcore's stack a:aws-library labels Oct 16, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.5%. Comparing base (cafbf96) to head (d5c816c).
Report is 651 commits behind head on master.

Files with missing lines Patch % Lines
...mq/rpc_interfaces/clusters_keeper/ec2_instances.py 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6551      +/-   ##
=========================================
+ Coverage    84.5%   87.5%    +2.9%     
=========================================
  Files          10    1256    +1246     
  Lines         214   54886   +54672     
  Branches       25    1243    +1218     
=========================================
+ Hits          181   48035   +47854     
- Misses         23    6641    +6618     
- Partials       10     210     +200     
Flag Coverage Δ
integrationtests 64.7% <ø> (?)
unittests 85.1% <96.4%> (+0.6%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ackages/aws-library/src/aws_library/ec2/_client.py 100.0% <100.0%> (ø)
...vice_integration/pytest_plugin/folder_structure.py 0.0% <ø> (ø)
...g/src/simcore_service_autoscaling/core/settings.py 100.0% <100.0%> (ø)
...e_service_autoscaling/modules/auto_scaling_core.py 89.3% <100.0%> (ø)
...mcore_service_clusters_keeper/rpc/ec2_instances.py 100.0% <100.0%> (ø)
...mq/rpc_interfaces/clusters_keeper/ec2_instances.py 0.0% <0.0%> (ø)

... and 1245 files with indirect coverage changes

@pcrespov pcrespov force-pushed the is6367/cluster-hybrid-machines branch from 866757a to deb557f Compare October 17, 2024 15:39
@pcrespov pcrespov changed the title WIP: Is6367/cluster hybrid machines ♻️ 📝 Minor refactor and doc of autoscaling service Oct 17, 2024
@pcrespov pcrespov marked this pull request as ready for review October 17, 2024 16:10
@pcrespov pcrespov requested a review from sanderegg as a code owner October 17, 2024 16:10
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍

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.

It's not just a documentation refactor it's change

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 for the first draft, but an empty EC2_INSTANCES_ALLOWED_TYPES means that no type is allowed. What your change does is to accept anything, which goes against how the system works.

@pcrespov
Copy link
Member Author

Thanks for the first draft, but an empty EC2_INSTANCES_ALLOWED_TYPES means that no type is allowed. What your change does is to accept anything, which goes against how the system works.

@sanderegg As I mentioned above in the description, this behaviour was already like this. I found it strange but I just did not want to change the logic. Now i see that probably you were not aware of this issue so let me show you and we can change it

@pcrespov pcrespov force-pushed the is6367/cluster-hybrid-machines branch from 59a70da to d2b2485 Compare October 18, 2024 13:36
@pcrespov pcrespov requested a review from sanderegg October 18, 2024 13:38
@pcrespov pcrespov enabled auto-merge (squash) October 18, 2024 13:47
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.

great thanks!

@pcrespov pcrespov force-pushed the is6367/cluster-hybrid-machines branch from dc22713 to c899568 Compare October 18, 2024 15:20
Copy link

@pcrespov pcrespov merged commit f9fffc8 into ITISFoundation:master Oct 21, 2024
57 checks passed
@pcrespov pcrespov deleted the is6367/cluster-hybrid-machines branch October 21, 2024 09:29
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 a:aws-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants