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.

Sign up for GitHub

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

Jump to bottom

feat: decouple function name from function domain #127

Merged
merged 3 commits into from Sep 24, 2020
Merged

feat: decouple function name from function domain #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f98519
decouple function name from function domain
Sep 22, 2020
66d5a42
addressing comments
Sep 22, 2020
13eecf7
handle path ending with '/'
Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 7 additions & 10 deletions client.go
View file
Open in desktop
Expand Up @@ -11,7 +11,6 @@ const (
DefaultRegistry = "docker.io"
DefaultRuntime = "go"
DefaultTrigger = "http"
DefaultMaxRecursion = 5 // when determining a name from path
)

// Client for managing Function instances.
Expand Down Expand Up @@ -127,7 +126,6 @@ func New(options ...Option) *Client {
lister: &noopLister{output: os.Stdout},
dnsProvider: &noopDNSProvider{output: os.Stdout},
progressListener: &noopProgressListener{},
domainSearchLimit: DefaultMaxRecursion, // no recursion limit deriving domain by default.
}

// Apply passed options, which take ultimate precidence.
Expand Down Expand Up @@ -306,6 +304,12 @@ func (c *Client) Create(cfg Function) (err error) {
// Initialize creates a new Function project locally using the settings
// provided on a Function object.
func (c *Client) Initialize(cfg Function) (err error) {

// Create project root directory, if it doesn't already exist
if err = os.MkdirAll(cfg.Root, 0755); err != nil {
return
}

Comment on lines +307 to +312
Copy link
Member

Choose a reason for hiding this comment

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

I really like the idea of creating the directory in which a function will be deployed because it would ensure the function is in a clean fresh directory. However, one small question: what do we do when a user wants to start over, without also removing their git history (.git) or other ancillary config files (.direnv)? This is why the code exists to check if a directory is effectively empty: contains no visible files, and no .faas.yaml, before allowing a re-run of create or init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am not mistaken, this command is not doing anything if the directory already exists (and contains some hidden files).

In that case user would do mkdir my_dir_with_hidden_files && faas init my_dir_with_hidden_files

// Create Function of the given root path.
f, err := NewFunction(cfg.Root)
if err != nil {
Expand All @@ -320,15 +324,8 @@ func (c *Client) Initialize(cfg Function) (err error) {

f.Image = cfg.Image

// Set the name to that provided, defaulting to path derivation if empty.
// Set the name to that provided.
f.Name = cfg.Name
if cfg.Name == "" {
f.Name = pathToDomain(f.Root, c.domainSearchLimit)
if f.Name == "" {
err = errors.New("Function name must be deriveable from path or explicitly provided")
return
}
}

// Assert runtime was provided, or default.
f.Runtime = cfg.Runtime
Expand Down
78 changes: 21 additions & 57 deletions client_test.go
View file
Open in desktop
Expand Up @@ -56,8 +56,14 @@ func TestCreateWritesTemplate(t *testing.T) {
// TestCreateInitializedAborts ensures that a directory which contains an initialized
// function does not reinitialize
func TestCreateInitializedAborts(t *testing.T) {
root := "testdata/example.com/testCreateInitializedAborts" // contains only a .faas.config
root := "testdata/example.com/testCreateInitializedAborts"
defer os.RemoveAll(root)

client := faas.New()
if err := client.Initialize(faas.Function{Root: root}); err != nil {
t.Fatal(err)
}

if err := client.Initialize(faas.Function{Root: root}); err == nil {
t.Fatal("error expected initilizing a path already containing an initialized Function")
}
Expand All @@ -67,6 +73,15 @@ func TestCreateInitializedAborts(t *testing.T) {
// files aborts.
func TestCreateNonemptyDirectoryAborts(t *testing.T) {
root := "testdata/example.com/testCreateNonemptyDirectoryAborts" // contains only a single visible file.
if err := os.MkdirAll(root, 0744); err != nil {
t.Fatal(err)
}
defer os.RemoveAll(root)
_, err := os.Create(root + "/file.txt")
if err != nil {
t.Fatal(err)
}

client := faas.New()
if err := client.Initialize(faas.Function{Root: root}); err == nil {
t.Fatal("error expected initilizing a Function in a nonempty directory")
Expand Down Expand Up @@ -212,57 +227,6 @@ func TestUnsupportedRuntime(t *testing.T) {
}
}

// TestDeriveDomain ensures that the name of the service is a domain derived
// from the current path if possible.
// see unit tests on the pathToDomain for more detailed logic.
func TestDeriveName(t *testing.T) {
// Create the root Function directory
root := "testdata/example.com/testDeriveDomain"
if err := os.MkdirAll(root, 0700); err != nil {
t.Fatal(err)
}
defer os.RemoveAll(root)

client := faas.New(faas.WithRepository(TestRepository))
if err := client.Create(faas.Function{Root: root}); err != nil {
t.Fatal(err)
}

f, err := faas.NewFunction(root)
if err != nil {
t.Fatal(err)
}

if f.Name != "testDeriveDomain.example.com" {
t.Fatalf("unexpected function name '%v'", f.Name)
}
}

// TestDeriveSubdomans ensures that a subdirectory structure is interpreted as
// multilevel subdomains when calculating a derived name for a service.
func TestDeriveSubdomains(t *testing.T) {
// Create the test Function root
root := "testdata/example.com/region1/testDeriveSubdomains"
if err := os.MkdirAll(root, 0700); err != nil {
t.Fatal(err)
}
defer os.RemoveAll(root)

client := faas.New(faas.WithRepository(TestRepository))
if err := client.Create(faas.Function{Root: root}); err != nil {
t.Fatal(err)
}

f, err := faas.NewFunction(root)
if err != nil {
t.Fatal(err)
}

if f.Name != "testDeriveSubdomains.region1.example.com" {
t.Fatalf("unexpected function name '%v'", f.Name)
}
}

// TestNamed ensures that an explicitly passed name is used in leau of the
// path derived name when provided, and persists through instantiations.
func TestNamed(t *testing.T) {
Expand Down Expand Up @@ -387,8 +351,8 @@ func TestDeriveImageDefaultRegistry(t *testing.T) {
func TestCreateDelegates(t *testing.T) {
var (
root = "testdata/example.com/testCreateDelegates" // .. in which to initialize
expectedName = "testCreateDelegates.example.com" // expected to be derived
expectedImage = "quay.io/alice/testCreateDelegates.example.com:latest"
expectedName = "testCreateDelegates" // expected to be derived
expectedImage = "quay.io/alice/testCreateDelegates:latest"
builder = mock.NewBuilder()
pusher = mock.NewPusher()
deployer = mock.NewDeployer()
Expand Down Expand Up @@ -501,8 +465,8 @@ func TestRun(t *testing.T) {
func TestUpdate(t *testing.T) {
var (
root = "testdata/example.com/testUpdate"
expectedName = "testUpdate.example.com"
expectedImage = "quay.io/alice/testUpdate.example.com:latest"
expectedName = "testUpdate"
expectedImage = "quay.io/alice/testUpdate:latest"
builder = mock.NewBuilder()
pusher = mock.NewPusher()
updater = mock.NewUpdater()
Expand Down Expand Up @@ -580,7 +544,7 @@ func TestUpdate(t *testing.T) {
func TestRemoveByPath(t *testing.T) {
var (
root = "testdata/example.com/testRemoveByPath"
expectedName = "testRemoveByPath.example.com"
expectedName = "testRemoveByPath"
remover = mock.NewRemover()
)

Expand Down
18 changes: 9 additions & 9 deletions cmd/create.go
View file
Open in desktop
Expand Up @@ -20,7 +20,6 @@ func init() {
createCmd.Flags().BoolP("confirm", "c", false, "Prompt to confirm all configuration options - $FAAS_CONFIRM")
createCmd.Flags().StringP("image", "i", "", "Optional full image name, in form [registry]/[namespace]/[name]:[tag] for example quay.io/myrepo/project.name:latest (overrides --repository) - $FAAS_IMAGE")
createCmd.Flags().StringP("namespace", "n", "", "Override namespace into which the Function is deployed (on supported platforms). Default is to use currently active underlying platform setting - $FAAS_NAMESPACE")
createCmd.Flags().StringP("path", "p", cwd(), "Path to the new project directory - $FAAS_PATH")
createCmd.Flags().StringP("repository", "r", "", "Repository for built images, ex 'docker.io/myuser' or just 'myuser'. Optional if --image provided. - $FAAS_REPOSITORY")
createCmd.Flags().StringP("runtime", "l", faas.DefaultRuntime, "Function runtime language/framework. - $FAAS_RUNTIME")
createCmd.Flags().StringP("templates", "", filepath.Join(configPath(), "templates"), "Extensible templates path. - $FAAS_TEMPLATES")
Expand All @@ -38,18 +37,18 @@ func init() {
}

var createCmd = &cobra.Command{
Use: "create <name>",
Use: "create <path>",
Short: "Create a new Function, including initialization of local files and deployment.",
SuggestFor: []string{"cerate", "new"},
PreRunE: bindEnv("image", "namespace", "path", "repository", "runtime", "templates", "trigger", "confirm"),
PreRunE: bindEnv("image", "namespace", "repository", "runtime", "templates", "trigger", "confirm"),
RunE: runCreate,
}

func runCreate(cmd *cobra.Command, args []string) (err error) {
config := newCreateConfig(args).Prompt()

function := faas.Function{
Name: config.Name,
Name: config.initConfig.Name,
Root: config.initConfig.Path,
Runtime: config.initConfig.Runtime,
Trigger: config.Trigger,
Expand Down Expand Up @@ -106,23 +105,24 @@ func newCreateConfig(args []string) createConfig {
}
}

// Prompt the user with value of config members, allowing for interaractive changes.
// Prompt the user with value of config members, allowing for interactive changes.
// Skipped if not in an interactive terminal (non-TTY), or if --confirm (agree to
// all prompts) was not explicitly set.
func (c createConfig) Prompt() createConfig {
name := deriveName(c.Name, c.initConfig.Path)
zroubalik marked this conversation as resolved.
Show resolved Hide resolved
if !interactiveTerminal() || !c.initConfig.Confirm {
// Just print the basics if not confirming
fmt.Printf("Project path: %v\n", c.initConfig.Path)
fmt.Printf("Project name: %v\n", name)
fmt.Printf("Function name: %v\n", c.initConfig.Name)
fmt.Printf("Runtime: %v\n", c.Runtime)
fmt.Printf("Trigger: %v\n", c.Trigger)
return c
}

derivedName, derivedPath := deriveNameAndAbsolutePathFromPath(prompt.ForString("Project path", c.initConfig.Path, prompt.WithRequired(true)))
lance marked this conversation as resolved.
Show resolved Hide resolved
return createConfig{
initConfig: initConfig{
Path: prompt.ForString("Project path", c.initConfig.Path),
Name: prompt.ForString("Project name", name, prompt.WithRequired(true)),
Name: derivedName,
Path: derivedPath,
Comment on lines -124 to +125
Copy link
Member

Choose a reason for hiding this comment

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

It appears to me that perhaps this omits the prompting of name and path entirely, right?

I was actuall fond of the original idea: to prompt if the parameter were not provided as a flag. If that's not the plan anymore, no problem! But if we wanted to prompt the user when the parameters had not been provided, would we not instead leave things roughly as they were, and insteaed add here a conditional that only prompts if the flag does not exist (as provided by the CLI API)?

Copy link
Member

Choose a reason for hiding this comment

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

It appears to me that perhaps this omits the prompting of name and path entirely, right?

@lkingland I may be misreading your comment, so disregard if so (or correct me). The prompt occurs on line 121.

derivedName, derivedPath := deriveNameAndAbsolutePathFromPath(prompt.ForString("Project path", c.initConfig.Path, prompt.WithRequired(true)))

Runtime: prompt.ForString("Runtime", c.Runtime),
Trigger: prompt.ForString("Trigger", c.Trigger),
// Templates intentionally omitted from prompt for being an edge case.
Expand Down
29 changes: 15 additions & 14 deletions cmd/init.go
View file
Open in desktop
Expand Up @@ -14,7 +14,6 @@ import (
func init() {
root.AddCommand(initCmd)
initCmd.Flags().BoolP("confirm", "c", false, "Prompt to confirm all configuration options - $FAAS_CONFIRM")
initCmd.Flags().StringP("path", "p", cwd(), "Path to the new project directory - $FAAS_PATH")
initCmd.Flags().StringP("runtime", "l", faas.DefaultRuntime, "Function runtime language/framework. - $FAAS_RUNTIME")
initCmd.Flags().StringP("templates", "", filepath.Join(configPath(), "templates"), "Extensible templates path. - $FAAS_TEMPLATES")
initCmd.Flags().StringP("trigger", "t", faas.DefaultTrigger, "Function trigger (ex: 'http','events') - $FAAS_TRIGGER")
Expand All @@ -25,10 +24,10 @@ func init() {
}

var initCmd = &cobra.Command{
Use: "init <name>",
Use: "init <path>",
Short: "Initialize a new Function project",
SuggestFor: []string{"inti", "new"},
PreRunE: bindEnv("path", "runtime", "templates", "trigger", "confirm"),
PreRunE: bindEnv("runtime", "templates", "trigger", "confirm"),
RunE: runInit,
// TODO: autocomplate Functions for runtime and trigger.
}
Expand All @@ -49,10 +48,10 @@ func runInit(cmd *cobra.Command, args []string) error {
}

type initConfig struct {
// Name of the service in DNS-compatible format (ex myfunc.example.com)
// Name of the Function.
Name string

// Path to files on disk. Defaults to current working directory.
// Absolute path to Function on disk.
Path string

// Runtime language/framework.
Expand All @@ -78,13 +77,15 @@ type initConfig struct {
// newInitConfig returns a config populated from the current execution context
// (args, flags and environment variables)
func newInitConfig(args []string) initConfig {
var name string
var path string
if len(args) > 0 {
name = args[0] // If explicitly provided, use.
path = args[0] // If explicitly provided, use.
}

derivedName, derivedPath := deriveNameAndAbsolutePathFromPath(path)
return initConfig{
Name: deriveName(name, viper.GetString("path")), // args[0] or derived
Path: viper.GetString("path"),
Name: derivedName,
Path: derivedPath,
Runtime: viper.GetString("runtime"),
Templates: viper.GetString("templates"),
Trigger: viper.GetString("trigger"),
Expand All @@ -96,19 +97,19 @@ func newInitConfig(args []string) initConfig {
// Skipped if not in an interactive terminal (non-TTY), or if --confirm false (agree to
// all prompts) was set (default).
func (c initConfig) Prompt() initConfig {
name := deriveName(c.Name, c.Path)
if !interactiveTerminal() || !c.Confirm {
// Just print the basics if not confirming
fmt.Printf("Project path: %v\n", c.Path)
fmt.Printf("Project name: %v\n", name)
fmt.Printf("Function name: %v\n", c.Name)
fmt.Printf("Runtime: %v\n", c.Runtime)
fmt.Printf("Trigger: %v\n", c.Trigger)
return c
}

derivedName, derivedPath := deriveNameAndAbsolutePathFromPath(prompt.ForString("Project path", c.Path, prompt.WithRequired(true)))
return initConfig{
// TODO: Path should be prompted for and set prior to name attempting path derivation. Test/fix this if necessary.
Path: prompt.ForString("Project path", c.Path),
Name: prompt.ForString("Project name", name, prompt.WithRequired(true)),
Name: derivedName,
Path: derivedPath,
Runtime: prompt.ForString("Runtime", c.Runtime),
Trigger: prompt.ForString("Trigger", c.Trigger),
// Templates intentiopnally omitted from prompt for being an edge case.
Expand Down
32 changes: 27 additions & 5 deletions cmd/root.go
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/mitchellh/go-homedir"
"github.com/ory/viper"
Expand Down Expand Up @@ -165,21 +166,42 @@ func functionWithOverrides(root, namespace, image string) (f faas.Function, err

// deriveName returns the explicit value (if provided) or attempts to derive
// from the given path. Path is defaulted to current working directory, where
// a function configuration, if it exists and contains a name, is used. Lastly
// derivation using the path us used.
// a Function configuration, if it exists and contains a name, is used.
func deriveName(explicitName string, path string) string {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this function could be better named as it no longer really derives the name, but instead just pulls it from the configuration file or returns the passed value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion?

Copy link
Member

@lance lance Sep 22, 2020 •
edited

Choose a reason for hiding this comment

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

Any suggestion?

I've been sitting here for 5 minutes trying to come up with a good name. I see why you asked. The best I've come up with is providedOrConfiguredName(provided, path string) string. But meh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, yeah 😄 I don't like the name either, so I am happy to change it to anything else, that makes sense for everyone

// If the name was explicitly provided, use it.
if explicitName != "" {
return explicitName
}

// If the directory at path contains an initialized Function, use the name therein
f, err := faas.NewFunction(path)
if err == nil && f.Name != "" {
return f.Name
}
maxRecursion := faas.DefaultMaxRecursion
derivedName, _ := faas.DerivedName(path, maxRecursion)
return derivedName

return ""
}

// deriveNameAndAbsolutePathFromPath returns resolved Function name and absolute path
// to the Function project root. The input parameter path could be one of:
// 'relative/path/to/foo', '/absolute/path/to/foo', 'foo' or ''
func deriveNameAndAbsolutePathFromPath(path string) (string, string) {
var absPath string

// If path is not specifed, we would like to use current working dir
if path == "" {
path = cwd()
}

// Expand the passed Function name to its absolute path
absPath, err := filepath.Abs(path)
if err != nil {
return "", ""
}

// Get the name of the Function, which equals to name of the current directory
pathParts := strings.Split(strings.TrimRight(path, string(os.PathSeparator)), string(os.PathSeparator))
return pathParts[len(pathParts)-1], absPath
}

// deriveImage returns the same image name which will be used if no explicit
Expand Down
2 changes: 1 addition & 1 deletion docs/commands.md
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@

## `init`

Creates a new Function project at _`path`_. If _`path`_ is unspecified, assumes the current directory. If _`path`_ does not exist, it will be created. The user can specify the runtime and trigger with flags.
Creates a new Function project at _`path`_. If _`path`_ is unspecified, assumes the current directory. If _`path`_ does not exist, it will be created. The function name is the name of the leaf directory at path. The user can specify the runtime and trigger with flags.

Similar `kn` command: none.

Expand Down