-
Notifications
You must be signed in to change notification settings - Fork 239
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
[Builder] Support configuring image name prefix template to enforce #1755
[Builder] Support configuring image name prefix template to enforce #1755
Conversation
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.
Looks great, minor comments.
What is stated in the description and what returned resolve_function_target_image_registries_to_enforce_prefix
is not aligned.
And just for clarity, we are opposed to only have one registry being set?
mlrun/builder.py
Outdated
IMAGE_NAME_ENRICH_REGISTRY_PREFIX = "." | ||
|
||
|
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.
PEP8 suggests that constants should be defined on a module level, so it would be more clear and visible which constants defined in the module.
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.
I'm not sure I understand what are you suggesting, a python file is a python module, this variable is defined in the global context of the file, meaning it is defined in the module level
Can you elaborate ?
tests/test_builder.py
Outdated
target_image = ( | ||
mlrun.builder.get_k8s_helper() | ||
.create_pod.call_args[0][0] | ||
.pod.spec.containers[0] | ||
.args[5] |
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.
I think it is clearer what that "5" is, if you assign it to a variable and use it.
target_image = ( | |
mlrun.builder.get_k8s_helper() | |
.create_pod.call_args[0][0] | |
.pod.spec.containers[0] | |
.args[5] | |
container_image_index = 5 | |
target_image = ( | |
mlrun.builder.get_k8s_helper() | |
.create_pod.call_args[0][0] | |
.pod.spec.containers[0] | |
.args[container_image_index] |
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.
While I agree having "magic numbers" in the code (usually it happens in tests) is not a good practice, here I don't think moving it to a variable helps.
A good example when moving something to a variable helps can be found in test_create_schedule_mlrun_function
Instead of having the number 1 everywhere, we assigned it to the expected_call_counter
, then we use it both to generate the trigger, and in the end to assert the number of runs triggered. The value from doing that is both explainability, and also if someone want to change this test to check trigger of 2 runs, they'll just need to change one value.
Here we have several magic numbers (the zeros are magic numbers too), they all derived from the arg index in the relevant object (the index of the container in the conatiners list, the index of the call to the function, the index of the arg in the function etc...). Those numbers will need to be changed if someone for example changes the function signature, but I don't think there is a way to connect the two (so signature change won't break the tests)
Bottomline I don't think variable helps here (there's nothing to explain about this number, it's just the index of the container image argument), I will move the extraction of the target image from the mock to a function so if signature changes we can change it in one place
…lrun#1755) (cherry picked from commit 1024299)
Adding a new config value under the
mlconf.httpdb.builder
block -function_target_image_name_prefix_template
which defaults tofunc-{project}-{name}
So far this default template was used when no target image was provided to generate the target image, from now on, in addition to that, if the target image is provided, and as long as it's targeted to the configured registry, we'll enforce the image name to have the configured prefix
To allow the UI to do the same verification added 2 new fields in the frontend spec response:
function_deployment_target_image_name_prefix_template
- which just sends the template (e.g.some-prefix-{project}-{name}-suffix
)function_deployment_target_image_registries_to_enforce_prefix
- which send the possible values for the registry prefix which if exists in the target image, the name should be verified to have the prefixFor example, let's say the configured registry is
registry.hub.docker.com/some-user
, the registries to enforce will beregistry.hub.docker.com/some-user/
and./some-user/
(.
marks us to enrich the registry), so for any target image that starts with this prefix (meaning this is its registry), the part after it (image name) must have the prefix according to the template