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

refactor!: change envVars to env in func.yaml #316

Merged
merged 1 commit into from
Apr 27, 2021
Merged

refactor!: change envVars to env in func.yaml #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func runDeploy(cmd *cobra.Command, _ []string) (err error) {
return
}

function.EnvVars = mergeEnvVarsMaps(function.EnvVars, config.EnvVars)
function.Env = mergeEnvMaps(function.Env, config.Env)

// Check if the Function has been initialized
if !function.Initialized() {
Expand Down Expand Up @@ -250,7 +250,7 @@ type deployConfig struct {
// Build the associated Function before deploying.
Build bool

EnvVars map[string]string
Env map[string]string
}

// newDeployConfig creates a buildConfig populated from command flags and
Expand All @@ -263,7 +263,7 @@ func newDeployConfig(cmd *cobra.Command) deployConfig {
Verbose: viper.GetBool("verbose"), // defined on root
Confirm: viper.GetBool("confirm"),
Build: viper.GetBool("build"),
EnvVars: envVarsFromCmd(cmd),
Env: envFromCmd(cmd),
}
}

Expand Down
19 changes: 10 additions & 9 deletions cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ package cmd
import (
"context"
"fmt"
"github.com/pkg/errors"
"os"
"path/filepath"
"strings"

"github.com/pkg/errors"

"github.com/mitchellh/go-homedir"
"github.com/ory/viper"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -252,25 +253,25 @@ func deriveImage(explicitImage, defaultRegistry, path string) string {
return derivedValue // Use the func system's derivation logic.
}

func envVarsFromCmd(cmd *cobra.Command) map[string]string {
envVarsM := make(map[string]string)
func envFromCmd(cmd *cobra.Command) map[string]string {
envM := make(map[string]string)
if cmd.Flags().Changed("env") {
envVarsA, err := cmd.Flags().GetStringArray("env")
envA, err := cmd.Flags().GetStringArray("env")
if err == nil {
for _, s := range envVarsA {
for _, s := range envA {
kvp := strings.Split(s, "=")
if len(kvp) == 2 && kvp[0] != "" {
envVarsM[kvp[0]] = kvp[1]
envM[kvp[0]] = kvp[1]
} else if len(kvp) == 1 && kvp[0] != "" {
envVarsM[kvp[0]] = ""
envM[kvp[0]] = ""
}
}
}
}
return envVarsM
return envM
}

func mergeEnvVarsMaps(dest, src map[string]string) map[string]string {
func mergeEnvMaps(dest, src map[string]string) map[string]string {
result := make(map[string]string, len(dest)+len(src))

for name, value := range dest {
Expand Down
7 changes: 4 additions & 3 deletions cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cmd

import (
"fmt"

"github.com/ory/viper"
"github.com/spf13/cobra"

Expand Down Expand Up @@ -44,7 +45,7 @@ func runRun(cmd *cobra.Command, args []string) (err error) {
return
}

function.EnvVars = mergeEnvVarsMaps(function.EnvVars, config.EnvVars)
function.Env = mergeEnvMaps(function.Env, config.Env)

err = function.WriteConfig()
if err != nil {
Expand Down Expand Up @@ -75,13 +76,13 @@ type runConfig struct {
// Verbose logging.
Verbose bool

EnvVars map[string]string
Env map[string]string
}

func newRunConfig(cmd *cobra.Command) runConfig {
return runConfig{
Path: viper.GetString("path"),
Verbose: viper.GetBool("verbose"), // defined on root
EnvVars: envVarsFromCmd(cmd),
Env: envFromCmd(cmd),
}
}
6 changes: 3 additions & 3 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type config struct {
Trigger string `yaml:"trigger"`
Builder string `yaml:"builder"`
BuilderMap map[string]string `yaml:"builderMap"`
EnvVars map[string]string `yaml:"envVars"`
Env map[string]string `yaml:"env"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@lance won't this cause issues for users using newer versions with older conf file?

Copy link
Contributor

Choose a reason for hiding this comment

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

But not many people use function so it is not that much of a problem. We just need to let know people that it's been changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - you are right that any existing users would need to change their config file. But this is all so new, and it's still unsupported, so I think perhaps adding to release notes for TP-2 would be OK instead of trying for backwards compatibility. But I will follow up on this to be sure it's OK.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!

Yes, this is technically a backwards incompatible change, but let's get this thing squeaky-clean now, before 1.0, while we still can. (I also doubt there are others using env vars other than we team members at this point?),

// Add new values to the toConfig/fromConfig functions.
}

Expand Down Expand Up @@ -60,7 +60,7 @@ func fromConfig(c config) (f Function) {
Trigger: c.Trigger,
Builder: c.Builder,
BuilderMap: c.BuilderMap,
EnvVars: c.EnvVars,
Env: c.Env,
}
}

Expand All @@ -75,7 +75,7 @@ func toConfig(f Function) config {
Trigger: f.Trigger,
Builder: f.Builder,
BuilderMap: f.BuilderMap,
EnvVars: f.EnvVars,
Env: f.Env,
}
}

Expand Down
11 changes: 6 additions & 5 deletions docker/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ package docker
import (
"context"
"fmt"
"os"
"strings"
"time"

"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/pkg/stdcopy"
"github.com/docker/go-connections/nat"
"github.com/pkg/errors"
"os"
"strings"
"time"

"github.com/docker/docker/client"

Expand Down Expand Up @@ -42,8 +43,8 @@ func (n *Runner) Run(ctx context.Context, f bosonFunc.Function) error {
return errors.New("Function has no associated Image. Has it been built?")
}

envs := make([]string, 0, len(f.EnvVars)+1)
for name, value := range f.EnvVars {
envs := make([]string, 0, len(f.Env)+1)
for name, value := range f.Env {
if !strings.HasSuffix(name, "-") {
envs = append(envs, fmt.Sprintf("%s=%s", name, value))
}
Expand Down
2 changes: 1 addition & 1 deletion function.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type Function struct {
// e.g. { "jvm": "docker.io/example/quarkus-jvm-builder" }
BuilderMap map[string]string

EnvVars map[string]string
Env map[string]string
}

// NewFunction loads a Function from a path on disk. use .Initialized() to determine if
Expand Down
24 changes: 12 additions & 12 deletions knative/deployer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (d *Deployer) Deploy(ctx context.Context, f bosonFunc.Function) (err error)
if d.Verbose {
fmt.Printf("Creating Knative Service: %v\n", f.Name)
}
service, err := generateNewService(f.Name, f.ImageWithDigest(), f.Runtime, f.EnvVars)
service, err := generateNewService(f.Name, f.ImageWithDigest(), f.Runtime, f.Env)
if err != nil {
err = fmt.Errorf("knative deployer failed to generate the service: %v", err)
return err
Expand Down Expand Up @@ -86,7 +86,7 @@ func (d *Deployer) Deploy(ctx context.Context, f bosonFunc.Function) (err error)
}
} else {
// Update the existing Service
err = client.UpdateServiceWithRetry(f.Name, updateService(f.ImageWithDigest(), f.EnvVars), 3)
err = client.UpdateServiceWithRetry(f.Name, updateService(f.ImageWithDigest(), f.Env), 3)
if err != nil {
err = fmt.Errorf("knative deployer failed to update the service: %v", err)
return err
Expand Down Expand Up @@ -114,7 +114,7 @@ func probeFor(url string) *corev1.Probe {
}
}

func generateNewService(name, image, runtime string, envVars map[string]string) (*servingv1.Service, error) {
func generateNewService(name, image, runtime string, env map[string]string) (*servingv1.Service, error) {
containers := []corev1.Container{
{
Image: image,
Expand Down Expand Up @@ -147,10 +147,10 @@ func generateNewService(name, image, runtime string, envVars map[string]string)
},
}

return setEnvVars(service, envVars)
return setEnv(service, env)
}

func updateService(image string, envVars map[string]string) func(service *servingv1.Service) (*servingv1.Service, error) {
func updateService(image string, env map[string]string) func(service *servingv1.Service) (*servingv1.Service, error) {
return func(service *servingv1.Service) (*servingv1.Service, error) {
// Removing the name so the k8s server can fill it in with generated name,
// this prevents conflicts in Revision name when updating the KService from multiple places.
Expand All @@ -160,7 +160,7 @@ func updateService(image string, envVars map[string]string) func(service *servin
if err != nil {
return service, err
}
return setEnvVars(service, envVars)
return setEnv(service, env)
}
}

Expand All @@ -187,22 +187,22 @@ func processValue(val string) (string, error) {
}
}

func setEnvVars(service *servingv1.Service, envVars map[string]string) (*servingv1.Service, error) {
builtEnvVarName := "BUILT"
builtEnvVarValue := time.Now().Format("20060102T150405")
func setEnv(service *servingv1.Service, env map[string]string) (*servingv1.Service, error) {
builtEnvName := "BUILT"
builtEnvValue := time.Now().Format("20060102T150405")

toUpdate := make(map[string]string, len(envVars)+1)
toUpdate := make(map[string]string, len(env)+1)
toRemove := make([]string, 0)

for name, value := range envVars {
for name, value := range env {
if strings.HasSuffix(name, "-") {
toRemove = append(toRemove, strings.TrimSuffix(name, "-"))
} else {
toUpdate[name] = value
}
}

toUpdate[builtEnvVarName] = builtEnvVarValue
toUpdate[builtEnvName] = builtEnvValue

for idx, val := range toUpdate {
v, err := processValue(val)
Expand Down