-
Notifications
You must be signed in to change notification settings - Fork 27
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
♻️ Enhance regex validation director v2 - Allow registries by IP and Port #2892
♻️ Enhance regex validation director v2 - Allow registries by IP and Port #2892
Conversation
services/director-v2/src/simcore_service_director_v2/models/schemas/constants.py
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2892 +/- ##
======================================
Coverage 79.5% 79.5%
======================================
Files 670 670
Lines 27737 27737
Branches 3220 3220
======================================
+ Hits 22063 22066 +3
+ Misses 4922 4921 -1
+ Partials 752 750 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- TIP: use composition with f-strings to make it more readable
- Make sure there are tests covering any new regex. It is specially critical for future changes (we can check together if you like)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use my suggestion.
The previous regex was totally broken.
As PC requested, also add tests.
services/director-v2/src/simcore_service_director_v2/models/schemas/constants.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for your contribution! :-)
ok with me but advice to implement changes we agreed .
SWARM_STACK_NAME="", | ||
DYNAMIC_SIDECAR_PROXY_SETTINGS=DynamicSidecarProxySettings(), | ||
) | ||
with pytest.raises(Exception) as exc_info: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValidationError
What do these changes do?
This makes a regex in the settings validation of director-v2 more permissive. The regex validates an env-var specifying the docker-registry which hosts the image for the dynamic sidecar.
Before the change, the following registry names were matched (and thus allowed):
Now, additionally registries specified by IP& Port (e.g.
10.0.0.1:5000
) are allowed.This change is necessary for deployments on multiple machines that run custom self-built development containers. For this multi-node setup, a self-hosted registry is necessary to deploy the images to the swarm.
For regex validation, see:
https://regex101.com/r/DoCQeV/1