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

Improving uniqueness of templateID string (via template name) #971

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cpfeiffer
Copy link

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

  • unit testcases were extended and pass
  • code has been used successfully in production for 10 months

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@cpfeiffer cpfeiffer requested a review from a team as a code owner June 1, 2023 10:54
Copy link
Member

@basil basil left a 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

@cpfeiffer
Copy link
Author

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.

@basil
Copy link
Member

basil commented Jun 1, 2023

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.

@basil
Copy link
Member

basil commented Jun 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong calculation of running Template agents when using the same image on multiple templates
3 participants