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

Sanitise user input for computerName #1

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

Conversation

uqperryk
Copy link

Added code will do the following to the computerName value provided by the user:

  • replace all spaces with hypens
  • drop all chars not a-z or a number or a hypen
  • reduce remaining to max 15 chars
  • sets to all lowercase

Should this code reduce the value to nothing, it will reset the computerName value to be the serial number.

Otherwise it will proceed with the value after sanitisation.

n.b I haven't tested this code yet, just a copy and paste from my script with the variable names changed back.

Also, this will default to a computer name which is simply the serial number of the computer if the user input is blank. I've seen these sorts of set name scripts do something like set a $prefix variable and then the computerName variable could become "$prefix-$serial".

Added code will do the following to the computerName value provided by the user:

- replace all spaces with hypens
- drop all chars not a-z or a number or a hypen
- reduce remaining to max 15 chars
- sets to all lowercase

Should this code reduce the value to nothing, it will reset the computerName value to be the serial number.

Otherwise it will proceed with the value after sanitisation.
Copy link
Owner

@neilmartin83 neilmartin83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thank you very much for this - I love the concept. Got some suggestions/feedback before I accept it.

Line 82 isn't needed as line 83 also removes spaces in my testing.

In my environment we have uppercase hostnames so line 85 does the opposite of what we want - I will definitely use a variation of that there but not sure if it's useful here as I can see people having different needs.

Also, line 85 - a nit pick of mine, backticks are deprecated - wrapping the command in $() is the new shiny. Also double quoting the variable is something I have a habit of doing - e.g. computerName=$(echo "$computerName" | tr a-z A-Z)

I'd double-quote $serial in line 90 as well :)

Thanks again!

The choice between uppercase or lowercase of everything may be better left to the individual admin who is implementing this workflow. This now by default does neither, however by uncommenting the correct line either option is available.

A couple of minor improvements of the bash code included too.
@uqperryk
Copy link
Author

No worries, I'm more than happy to be able to contribute back to your workflow.

Removing what was line 83 (the first sanitisation) is actually a little different to keeping it, the difference is that spaces become hyphens rather than being removed. There are pros and cons to this, pro is I suspect there will be cases of " " when they meant "-", however a con is that you can end up with cases where a bunch of extra spaces was accidentally added and instead of dropping these they end up hyphens.

I've left it in for the moment, but obviously feel free to make your own decision on all of this. I won't mind in any way. The idea was simply to convey the concept to you and show you how we did it.

I've implemented a couple of your other improvements to the bash code, completely agree with these and I'm aware the line with the backticks was something borrowed from a fairly old script. I've additionally added another improvement suggested by shellcheck to use tr '[:upper:]' '[:lower:]' rather than tr A-Z a-z. In this case it makes no difference however I guess you never know when the code may be used elsewhere.

I'm with you on the uppercase/lowercase/change nothing choice being something that everyone will have their own requirements for and is a little too opinionated for this setup to choose. I've changed this so that case isn't changed at all however included are the lines needed to do so if anyone wishes to un-comment these as needed.

Cheers.

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

Successfully merging this pull request may close these issues.

2 participants