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

snap/naming: move various name validation helpers to separate package #6016

Merged

Conversation

bboozzoo
Copy link
Collaborator

Follow-up on the package separation idea discussed in #5948. The PR moves various snap related name validation helpers into a separate package that can be easily imported without pulling in the whole of githut.com/snapcore/snapd/snap baggage.

As part of the PR, the package is used by model assertion validation code, where the snap package could not have been previously used.

Introuduce a new package that pulls out the validation of various names around
snaps. Name validation covers: snaps, instances, apps, aliases, hooks,
slots/plugs/interfaces, sockets.

The package has no extra dependencies.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@chipaca
Copy link
Contributor

chipaca commented Oct 18, 2018

wouldn't snap/validate with e.g. validate.Plug() functions, be nicer to use?

(super big +1 to the change beyond this)

@bboozzoo
Copy link
Collaborator Author

@chipaca that's something I thought of in the beginning. The downside is, you'd still need to have access to snap.Info to move the whole of validation code into a separate package, so validate would need to import snap or some other package that brings Info (snap/meta?). On the other hand, I'm really only interested in having access to snap.ValidateName() and maybe ValidateInstanceName() from asserts. I'm open to suggestions though. Maybe we could move the 'heavy' code out of snap?

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

The change itself looks fine. I don't have any strong opinions either way about this move so +1 to unblock progress.

@mvo5
Copy link
Contributor

mvo5 commented Nov 9, 2018

I think this is nice - I am not sure if I understand @chipaca correct but I also prefer snap/validate' over snap/nameorsnap/validatenames (but thats a bit heavy). I mean, right now it seems all the exported functions are Validate{Instance,Snap,Plug,Slot,Hook,Alias,App,Socket} so why not validate.SnapName() or something along these lines?

@zyga
Copy link
Collaborator

zyga commented Dec 14, 2018

Please deconflict @bboozzoo

@pedronis pedronis self-requested a review December 20, 2018 08:52
Rename the package

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo
Copy link
Collaborator Author

As discussed, I've renamed the package to snap/naming. The changed code reads surprisingly nice.

@bboozzoo bboozzoo changed the title [RFC] move various name validation helpers to snap/name package snap/naming: move various name validation helpers to separate package Jan 25, 2019
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Contributor

@chipaca chipaca left a comment

Choose a reason for hiding this comment

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

I like.

return fmt.Errorf("invalid instance key: %q", instanceKey)
}
return nil
return naming.ValidateInstance(instanceName)
Copy link
Collaborator

@pedronis pedronis Jan 25, 2019

Choose a reason for hiding this comment

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

what are your thoughts? is that a intermediate step for churn? I would suppose in the end we don't want this indirection functions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I didn't want the diff to be overly large. The cleanup can be introduced once this PR lands.

@pedronis pedronis self-requested a review January 25, 2019 09:47
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you, +1 with some minor but relevant comments (some are old stuff spotted now because of the move)

}
if !validInstanceKey.MatchString(instanceKey) {
// TODO parallel-install: extend the error message once snap
// install help has been updated
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this TODO still relevant? something we can do now?

// validators.
var almostValidName = regexp.MustCompile("^[a-z0-9-]*[a-z][a-z0-9-]*$")

// validInstanceKey is a regular expression describing valid snap instance key
Copy link
Collaborator

Choose a reason for hiding this comment

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

a valid

*
*/

package naming
Copy link
Collaborator

Choose a reason for hiding this comment

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

// Package naming implements naming constraints for snaps and their elements.

// validInstanceKey is a regular expression describing valid snap instance key
var validInstanceKey = regexp.MustCompile("^[a-z0-9]{1,10}$")

// isValidName checks snap and socket socket identifiers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/socket socket/socket/ ?

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo bboozzoo merged commit 291b14b into snapcore:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants