-
Notifications
You must be signed in to change notification settings - Fork 318
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
Improving uniqueness of templateID string (via template name) #971
base: master
Are you sure you want to change the base?
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.
I sometimes see pull requests that solve a problem well enough for a particular user but not well enough for the general case of all users. This is one of those PRs. This should be generalized further. The problem is you are relying on the name being unique, which is true in your case but may not be true for other users. Documenting the limitation is a workaround for the fact that the name is not guaranteed to be unique. It would be better to change the system to strongly encourage unique names for all users:
- Print a form validation warning when saving a template whose name is blank or not unique
- Log a warning to the build log when creating a container from a template whose name is blank or not unique
Fair enough. We chose to be defensive here. So far, the template name has been completely optional and defaulted to just "docker". We didn't change that behavior, but gave the user an option to fix this issue. Suddenly asking this to be unique might upset a few of the 38k users of this plugin. I wouldn't mind, though. |
To avoid bothering people who do not need to be bothered, we could only issue a warning when (a) there is more than one template and (b) two templates resolve to the same effective value (e.g., two blank templates, one blank template and one template named "docker", two templates named "docker", two templates named "my-template", etc). The code depends on this for correctness, and you are warning people about it in the help, but I think the warning should be more prominent than that to avoid supportability challenges. I do not think this is a big ask since you are already pushing for this in the help text. |
FYI, I have removed myself from the list of maintainers for this plugin, so even if the requested changes are made and I approve this PR, I would still not have permissions to do a release. If this PR is important to you, I would recommend adopting this plugin. |
Just as in #816 and #874, we want to distinguish templates that use the same docker image.
We do this pragmatically by using the user-supplied template name, so that the user is in control to make it unique.
So this fixes #816 as long as the user provides distinguished template names, which is explained by the help texts.
Testing done
Submitter checklist