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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 (鈿狅笍 devops) Fix validation of DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS: Make it conform to docker specs #3190

Conversation

mrnicegyu11
Copy link
Member

@mrnicegyu11 mrnicegyu11 commented Jul 14, 2022

What do these changes do?

Allow underscore in DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS in regex validation during container start

Before:
node.labels.standard_worker==true --> invalid
node.labels.standardworker==true --> valid

Now:
node.labels.standard_worker==true --> valid
node.labels.standardworker==true --> valid

(鈿狅笍 devops)

@mrnicegyu11 mrnicegyu11 added bug buggy, it does not work as expected a:director issue related with the director service a:director-v2 issue related with the director-v2 service labels Jul 14, 2022
@mrnicegyu11 mrnicegyu11 self-assigned this Jul 14, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #3190 (f15c49e) into master (06ed666) will increase coverage by 0.9%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3190     +/-   ##
========================================
+ Coverage    81.0%   82.0%   +0.9%     
========================================
  Files         740     740             
  Lines       31572   31572             
  Branches     4081    4081             
========================================
+ Hits        25602   25892    +290     
+ Misses       5147    4846    -301     
- Partials      823     834     +11     
Flag Coverage 螖
integrationtests 66.5% <酶> (+4.7%) 猬嗭笍
unittests 78.5% <酶> (-0.1%) 猬囷笍

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

Impacted Files Coverage 螖
...2/src/simcore_service_director_v2/core/settings.py 96.5% <酶> (酶)
...rvice_datcore_adapter/utils/requests_decorators.py 68.4% <0.0%> (-10.6%) 猬囷笍
...ector_v2/modules/comp_scheduler/background_task.py 83.3% <0.0%> (-8.4%) 猬囷笍
...mcore_service_webserver/garbage_collector_utils.py 82.0% <0.0%> (-2.6%) 猬囷笍
...service_director_v2/api/routes/dynamic_services.py 87.2% <0.0%> (+0.9%) 猬嗭笍
...c/simcore_service_catalog/core/background_tasks.py 68.4% <0.0%> (+2.1%) 猬嗭笍
.../simcore_service_catalog/db/repositories/groups.py 75.6% <0.0%> (+2.7%) 猬嗭笍
...or_v2/models/schemas/dynamic_services/scheduler.py 98.4% <0.0%> (+3.0%) 猬嗭笍
...rc/simcore_service_director_v2/core/application.py 90.4% <0.0%> (+3.1%) 猬嗭笍
...ector_v2/modules/dynamic_sidecar/scheduler/task.py 80.4% <0.0%> (+3.9%) 猬嗭笍
... and 10 more

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

pair reviewed. Expected test as agreed. thx

@@ -157,6 +157,11 @@ def test_expected_failure_dynamic_sidecar_settings(
('["two!=no"]', ["two!=no"]),
('["one==yes", "two!=no"]', ["one==yes", "two!=no"]),
('[" strips.white.spaces == ok "]', ["strips.white.spaces == ok"]),
(
# Bug from https://github.com/ITISFoundation/osparc-simcore/pull/3190
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this comment, it's fine like this

Copy link
Contributor

Choose a reason for hiding this comment

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

just make sure to remove this comment

(
# Bug from https://github.com/ITISFoundation/osparc-simcore/pull/3190
'["node.labels.standard_worker==true"]',
["node.labels.standard_worker==true"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add the following test case

('["underscore_label==underscore_value"]', ["underscore_label==underscore_value"]),

@GitHK
Copy link
Contributor

GitHK commented Jul 14, 2022

Since you are changing these, please also add the above suggestions. Thanks!

@mrnicegyu11
Copy link
Member Author

Since you are changing these, please also add the above suggestions. Thanks!

I intentionally did not, it is unclear and untested if this is even permitted from docker.
Let's keep the changes small and atomic and not overdo it.
This change currently prevents master from booting, so I would be happy if it gets in @GitHK

@mrnicegyu11 mrnicegyu11 requested a review from GitHK July 14, 2022 13:44
@GitHK
Copy link
Contributor

GitHK commented Jul 14, 2022

Since you are changing these, please also add the above suggestions. Thanks!

I intentionally did not, it is unclear and untested if this is even permitted from docker. Let's keep the changes small and atomic and not overdo it. This change currently prevents master from booting, so I would be happy if it gets in @GitHK

Let's have a look at the docs.

Apparently the key part of the label cannot contain underscores but only alphanumerical character and . and -. Please replace the _ with - in the tests and in the regex.

Regarding the value part of the label, all valid json encodable characters are ok. So _ is ok as well.

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.

馃憤 Thanks for taking care of this

@@ -157,6 +157,11 @@ def test_expected_failure_dynamic_sidecar_settings(
('["two!=no"]', ["two!=no"]),
('["one==yes", "two!=no"]', ["one==yes", "two!=no"]),
('[" strips.white.spaces == ok "]', ["strips.white.spaces == ok"]),
(
# Bug from https://github.com/ITISFoundation/osparc-simcore/pull/3190
Copy link
Contributor

Choose a reason for hiding this comment

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

just make sure to remove this comment

@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2022

Kudos, SonarCloud Quality Gate passed!聽 聽 Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mrnicegyu11 mrnicegyu11 changed the title 馃悰 Allow underscore in DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS 馃悰 Fix validation of DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS: Make it conform to docker specs Jul 19, 2022
@mrnicegyu11 mrnicegyu11 merged commit 5ead5b0 into ITISFoundation:master Jul 19, 2022
@mrnicegyu11 mrnicegyu11 deleted the fix/enableUnderscoresInRegexValidation branch July 19, 2022 14:46
@mrnicegyu11 mrnicegyu11 changed the title 馃悰 Fix validation of DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS: Make it conform to docker specs 馃悰 (鈿狅笍 devops) Fix validation of DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS: Make it conform to docker specs Jul 20, 2022
@sanderegg sanderegg added this to the Meteora milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director issue related with the director service a:director-v2 issue related with the director-v2 service bug buggy, it does not work as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants