Skip to content

FnCLI fn deploy #670

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions commands/deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,9 +390,12 @@ func (p *deploycmd) deployFuncV20180708(c *cli.Context, app *models.App, funcfil
if !p.local {
// fetch the architectures
shape = app.Shape
fmt.Printf("*****shape is %v *******", shape)

Choose a reason for hiding this comment

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

Can we print this log once the shape is properly assigned (after default case ie., line 400)?

if shape == "" {
fmt.Printf("shape is default")
shape = common.DefaultAppShape
app.Shape = shape
//shape = "GENERIC_ARM"
}

if _, ok := common.ShapeMap[shape]; !ok {
Expand Down
101 changes: 85 additions & 16 deletions common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"os/exec"
"os/signal"
"path/filepath"
"runtime"
"strings"
"time"
"unicode"
Expand All @@ -55,6 +56,7 @@ const (
MinRequiredDockerVersion = "17.5.0"
BuildxBuilderInstance = "oci_fn_builder"
DefaultAppShape = modelsv2.AppShapeGENERICX86
containerEngineTypeDocker = "docker"
)

var GlobalVerbose bool
Expand All @@ -63,7 +65,13 @@ var CommandVerbose bool
var ShapeMap = map[string][]string{
modelsv2.AppShapeGENERICX86: {"linux/amd64"},
modelsv2.AppShapeGENERICARM: {"linux/arm64"},
modelsv2.AppShapeGENERICX86ARM: {"linux/amd64", "linux/arm64"},
modelsv2.AppShapeGENERICX86ARM: {"linux/arm64", "linux/amd64"},
}

var TargetPlatformMap = map[string][]string{
modelsv2.AppShapeGENERICX86: {"amd64"},
modelsv2.AppShapeGENERICARM: {"arm64"},
modelsv2.AppShapeGENERICX86ARM: {"amd64_arm64"},
}

func IsVerbose() bool {
Expand Down Expand Up @@ -408,15 +416,18 @@ func containerEngineBuildV20180708(verbose bool, fpath string, ff *FuncFileV2018
return nil
}

func buildXDockerCommand(imageName, dockerfile string, buildArgs []string, noCache bool, architectures []string) []string {
func buildXDockerCommand(imageName, dockerfile string, buildArgs []string, noCache bool, architectures []string, containerEngineType string) []string {
var buildCommand = "buildx"
var name = imageName

plat:= strings.Join(architectures,"," )

Choose a reason for hiding this comment

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

plat is not comprehensive. You can directly use 'strings.Join' command in fmt.Println()
Another variable is not required.

Buildx platform confusing. You can say 'Specified Platform/s : '

fmt.Println("buildX platform is %v", plat)
args := []string{
buildCommand,
"build",
"-t", name,
//"-t", name,

Choose a reason for hiding this comment

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

Can we clean these comments?

"-f", dockerfile,
//"--load",
"--platform", strings.Join(architectures, ","),
}

Expand All @@ -435,6 +446,15 @@ func buildXDockerCommand(imageName, dockerfile string, buildArgs []string, noCac
var label = "imageName=" + imageName
args = append(args, "--build-arg", arg)
args = append(args, "--label", label)
}

if containerEngineType != containerEngineTypeDocker {
fmt.Println("*** engine type not docker append load")

Choose a reason for hiding this comment

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

Can these be in 'DEBUG' logs? It will be too 'internal' for the INFO logs.

args = append(args, "--manifest", name)
args = append(args, "--load")
} else {
fmt.Println("*** engine type docker append push")
args = append(args, "-t", name)
args = append(args, "--push")
}

Expand All @@ -447,12 +467,16 @@ func buildXDockerCommand(imageName, dockerfile string, buildArgs []string, noCac
return args
}

func buildDockerCommand(imageName, dockerfile string, buildArgs []string, noCache bool) []string {
func buildDockerCommand(imageName, dockerfile string, buildArgs []string, noCache bool, architectures []string) []string {
var name = imageName
plat:= strings.Join(architectures,"," )
fmt.Println("build platform is %v", plat)

Choose a reason for hiding this comment

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

plat is not comprehensive. You can directly use 'strings.Join' command in fmt.Println()
Another variable is not required.

Build platform confusing. You can say 'Specified Platform/s : '

Choose a reason for hiding this comment

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

Make the logs look more professional


args := []string{
"build",
"-t", name,
"-f", dockerfile,
"--platform", strings.Join(architectures, ","),
}

if noCache {
Expand All @@ -476,6 +500,7 @@ func buildDockerCommand(imageName, dockerfile string, buildArgs []string, noCach

// RunBuild runs function from func.yaml/json/yml.
func RunBuild(verbose bool, dir, imageName, dockerfile string, buildArgs []string, noCache bool, containerEngineType string, shape string) error {
var issuePush bool
cancel := make(chan os.Signal, 3)
signal.Notify(cancel, os.Interrupt) // and others perhaps
defer signal.Stop(cancel)
Expand Down Expand Up @@ -511,23 +536,38 @@ func RunBuild(verbose bool, dir, imageName, dockerfile string, buildArgs []strin

go func(done chan<- error) {
var dockerBuildCmdArgs []string

// Depending whether architecture list is passed or not trigger docker buildx or docker build accordingly
var mappedArchitectures []string

if arch, ok := ShapeMap[shape]; ok {
var mappedArchitectures []string
mappedArchitectures = append(mappedArchitectures, arch...)
err := initializeContainerBuilder(containerEngineType, mappedArchitectures)
if err != nil {
done <- err
return
var hostedPlatform = runtime.GOARCH
if platform, ok := TargetPlatformMap[shape]; ok {
// create target platform string to compare with hosted platform
targetPlatform := strings.Join(platform," ")

Choose a reason for hiding this comment

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

Why are we appending " " to platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment in the code

fmt.Println("hosted platform %v target platform %v", hostedPlatform, targetPlatform)

Choose a reason for hiding this comment

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

Format the log statement

if targetPlatform != hostedPlatform {
fmt.Println("TargetedPlatform and hostPlatform are not same")

Choose a reason for hiding this comment

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

This can be inferred from the log at line 549. Do we need it again?
As the logs are public facing, lets limit the INFO logs. If its needed for DEBUG, lets put it in DEBUG logs

err := initializeContainerBuilder(containerEngineType, mappedArchitectures)
if err != nil {
done <- err
return
}
dockerBuildCmdArgs = buildXDockerCommand(imageName, dockerfile, buildArgs, noCache, mappedArchitectures, containerEngineType )
// perform cleanup
defer cleanupContainerBuilder(containerEngineType)
} else {
fmt.Println("TargetedPlatform and hostPlatform are same")
fmt.Println("1. issuePush is ", issuePush)

Choose a reason for hiding this comment

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

Do we need these issuePush logs?

dockerBuildCmdArgs = buildDockerCommand(imageName, dockerfile, buildArgs, noCache, mappedArchitectures )
issuePush = true
fmt.Println("2. issuePush is ", issuePush)
}
}

dockerBuildCmdArgs = buildXDockerCommand(imageName, dockerfile, buildArgs, noCache, mappedArchitectures)
// perform cleanup
defer cleanupContainerBuilder(containerEngineType)
} else {
dockerBuildCmdArgs = buildDockerCommand(imageName, dockerfile, buildArgs, noCache)
}

fmt.Println("*****containerEngineType*****",containerEngineType )

Choose a reason for hiding this comment

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

Can we merge it into a single log?

fmt.Println("*****dockerBuildCmdArgs*******", dockerBuildCmdArgs)
cmd := exec.Command(containerEngineType, dockerBuildCmdArgs...)
cmd.Dir = dir
cmd.Stderr = buildErr // Doesn't look like there's any output to stderr on docker build, whether it's successful or not.
Expand All @@ -551,6 +591,31 @@ func RunBuild(verbose bool, dir, imageName, dockerfile string, buildArgs []strin
fmt.Fprintln(os.Stderr)
return fmt.Errorf("build cancelled on signal %v", signal)
}
if containerEngineType != containerEngineTypeDocker || issuePush {
fmt.Println("Using Container engine", containerEngineType, "to push as --push id only for docker and --load is for podman")

Choose a reason for hiding this comment

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

Optimise these logs.

Log statement need not be true because we use push even for podman with Build command

// Push to docker registry
fmt.Println("Using Container engine", containerEngineType, "to push")
fmt.Printf("Pushing %v to docker registry...", imageName)
if issuePush == true {
// build push
fmt.Println("***build push***")
cmd := exec.Command(containerEngineType, "push", imageName)
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout
if err := cmd.Run(); err != nil {
return fmt.Errorf("error running %v push, are you logged?: %v", containerEngineType, err)
}
} else {
// buildX push for podman
fmt.Println("***buildX push for podman***")
cmd := exec.Command(containerEngineType, "manifest", "push", imageName)
cmd.Stderr = os.Stderr
cmd.Stdout = os.Stdout
if err := cmd.Run(); err != nil {
return fmt.Errorf("error running %v push, are you logged?: %v", containerEngineType, err)
}
}
}
return nil
}

Expand Down Expand Up @@ -620,6 +685,10 @@ func isSupportedByDefaultBuildxPlatforms(containerEngineType string, platforms [

func initializeContainerBuilder(containerEngineType string, platforms []string) error {

if containerEngineType != containerEngineTypeDocker {
fmt.Println("engine type not docker return nil")
return nil
}
if isSupportedByDefaultBuildxPlatforms(containerEngineType, platforms) {
return nil
}
Expand Down
7 changes: 6 additions & 1 deletion objects/app/apps.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,11 @@ func (a *appsCmd) create(c *cli.Context) error {
}

appWithFlags(c, app)

// If architectures flag is not set then default it to nil
if c.IsSet(SHAPE_PARAMETER) {
shapeParam := c.String(SHAPE_PARAMETER)

fmt.Printf("in apps.go shape parameter is set and is %v", shapeParam)
// Check for architectures parameter passed or set to default
if len(shapeParam) == 0 {
return errors.New("no shape specified for the application")
Expand All @@ -165,6 +166,8 @@ func (a *appsCmd) create(c *cli.Context) error {
return errors.New("invalid shape specified for the application")
}
app.Shape = shapeParam
} else {
fmt.Printf("in apps.go shape parameter is not set")
}
_, err := CreateApp(a.client, app)
return err
Expand All @@ -178,12 +181,14 @@ func CreateApp(a *fnclient.Fn, app *modelsv2.App) (*modelsv2.App, error) {
})

if err != nil {

switch e := err.(type) {
case *apiapps.CreateAppBadRequest:
err = fmt.Errorf("%v", e.Payload.Message)
case *apiapps.CreateAppConflict:
err = fmt.Errorf("%v", e.Payload.Message)
}
//fmt.Println("create App error is %v", err)
return nil, err
}
fmt.Println("Successfully created app: ", resp.Payload.Name)
Expand Down
1 change: 1 addition & 0 deletions objects/app/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func Create() cli.Command {
Usage: "Syslog URL to send application logs to",
},
cli.StringFlag{
//cli.StringSliceFlag{
Name: "shape",
Usage: "Valid values are GENERIC_X86, GENERIC_ARM and GENERIC_X86_ARM. Default is GENERIC_X86. Setting this to GENERIC_X86, will run the functions in the application on X86 processor architecture.\n Setting this to GENERIC_ARM, will run the functions in the application on ARM processor architecture.\n When set to 'GENERIC_X86_ARM', functions in the application are run on either X86 or ARM processor architecture.\n Accepted values are:\n GENERIC_X86, GENERIC_ARM, GENERIC_X86_ARM",
},
Expand Down