Added support for named-servers#78
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
|
Hello! Thank you for opening this :) I think expanding {servername} is probably the way to go. Maybe steal this code from kubespawner? Not sure if we need the legacy support. We can then set the default template. |
|
I changed it to use |
GeorgianaElena
left a comment
GeorgianaElena
left a comment
There was a problem hiding this comment.
Awesome work @scj643, thank you! 🎉 It would be great if you could also update the README, so that folks know about the named servers support? (maybe here).
@yuvipanda, the readme says that unit_name_template should contain only [a-zA-Z0-9_-]. What happens if it doesn't (let's say because of servername)? Should we ensure this restriction by using something like escapism, similar with what kubespawner does here?
|
That's actually a good catch, @GeorgianaElena! Using escapism seems the right way to do it. |
GeorgianaElena
left a comment
GeorgianaElena
left a comment
There was a problem hiding this comment.
Thank you, @yuvipanda :)
@scj643, do you want to make sure we're having a unit name that respects the restriction in the readme by using escapism? ❤️
It should be fairly similar with what happens in kubespawner, with the difference that an _ is a safe character for the systemd unit name:
Added checking for string length when escaping. Added option for striping trailing character for _expand_user_vars. Made escape_char configurable.
| unit_trailing_character = Unicode( | ||
| '-', | ||
| help=""" | ||
| When using a unit that ends in a SERVERNAME strip this character from the end of the parsed string. | ||
| """ | ||
| ).tag(config=True) | ||
|
|
||
| escape_char = Unicode( | ||
| ':', | ||
| help=""" | ||
| The character to change invalid safe characters to. | ||
| """ | ||
| ).tag(config=True) |
There was a problem hiding this comment.
Making these configurable might open up the room for possible miss configurations. Let's just add this info in the help description of unit_name_template.
| Template to use to make the systemd service names. | ||
|
|
||
| {USERNAME} and {USERID} are expanded} | ||
| {USERNAME}, {SERVERNAME}, and {USERID} are expanded} |
There was a problem hiding this comment.
{USERID} has been replaced by {USERNAME} for the unit name, so it can be removed as it's not longer used.
| if len(in_string) > self.max_unit_length: | ||
| self.log.warning(f'String is longer than {self.max_unit_length}') | ||
| # Slice the string to the max unit length | ||
| return escapism.escape(in_string[:self.max_unit_length], safe=self.safe_chars, escape_char=self.escape_char) |
There was a problem hiding this comment.
I was thinking that rather than slice the unit name ourselves if it exceeds max length, we should instead throw an error if this happens.
The reason why I think this would be better is because if we slice it, the unit name will not respect the template anymore, and one might end up with duplicates.
| USERNAME=self.user.name, | ||
| USERID=self.user.id | ||
| ) | ||
| USERID=self.user.id, |
There was a problem hiding this comment.
USERID can be removed from here also, as it's not used anymore for the unit name.
| ).tag(config=True) | ||
|
|
||
| # Characters that are safe for systemd units. | ||
| safe_chars = set(string.ascii_lowercase + string.digits + string.ascii_uppercase + ':-_.\\') |
There was a problem hiding this comment.
The escape character cannot be in the list of safe chars. Not sure if this causes an error or a warning, but it's best to remove it.
|
Is there any chance that someone will take on this PR again? |
5 participants
Append the server name to the unit to allow named servers. This closes #73
This could use a better way of handling templating.