Skip to content

Commit

Permalink
Build command first refactor (#4189)
Browse files Browse the repository at this point in the history
* Build command and logic refactor POC

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fixed some conflict in go.mod and remove some commented piece of code

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Added file headers to run unit tests

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Removed remote builder and use directly OktetoBuilder in pkg/cmd/build for remote deploy/destroy. They don't need the wrapper V1 or v2

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fixed golangci and deepsource issues

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fixed unit and e2e tests. Added context to remote destroy

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Removed debug logging and fix log line

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Add test for getOriginalCWD and fix destroy remote when the manifest is in another path

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fix unix tests

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fix windows unit tests for function getOriginalCWD

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Addressed code review comments: Removed commented code and added an init function for the OktetoBuilder

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

* Fix warning in DeepSource

Signed-off-by: Nacho Fuertes <nacho@okteto.com>

---------

Signed-off-by: Nacho Fuertes <nacho@okteto.com>
  • Loading branch information
ifbyol committed Mar 5, 2024
1 parent f58d268 commit fb49fc0
Show file tree
Hide file tree
Showing 15 changed files with 650 additions and 492 deletions.
81 changes: 44 additions & 37 deletions cmd/build/remote/build.go → cmd/build/basic/build.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2023 The Okteto Authors
// Copyright 2024 The Okteto Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
Expand All @@ -11,65 +11,56 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package remote
package basic

import (
"context"
"fmt"
"path/filepath"
"strings"

"github.com/okteto/okteto/cmd/utils"
"github.com/okteto/okteto/pkg/analytics"
buildCmd "github.com/okteto/okteto/pkg/cmd/build"
"github.com/okteto/okteto/pkg/env"
oktetoErrors "github.com/okteto/okteto/pkg/errors"
"github.com/okteto/okteto/pkg/filesystem"
"github.com/okteto/okteto/pkg/log/io"
"github.com/okteto/okteto/pkg/okteto"
"github.com/okteto/okteto/pkg/registry"
"github.com/okteto/okteto/pkg/types"
"github.com/spf13/afero"
)

// OktetoBuilderInterface runs the build of an image
type oktetoBuilderInterface interface {
// BuildRunner runs the build of an image
type BuildRunner interface {
Run(ctx context.Context, buildOptions *types.BuildOptions, ioCtrl *io.Controller) error
}

type OktetoRegistryInterface interface {
GetImageTagWithDigest(imageTag string) (string, error)
IsOktetoRegistry(image string) bool
HasGlobalPushAccess() (bool, error)
IsGlobalRegistry(image string) bool

GetRegistryAndRepo(image string) (string, string)
GetRepoNameAndTag(repo string) (string, string)
}

// OktetoBuilder builds the images
type OktetoBuilder struct {
Builder oktetoBuilderInterface
Registry OktetoRegistryInterface
ioCtrl *io.Controller
// Builder It provides basic functionality to build images.
// This might be used as a base for more complex builders (e.g. v1, v2)
type Builder struct {
BuildRunner BuildRunner
IoCtrl *io.Controller
}

// NewBuilderFromScratch creates a new okteto builder
func NewBuilderFromScratch(ioCtrl *io.Controller) *OktetoBuilder {
builder := &buildCmd.OktetoBuilder{
OktetoContext: &okteto.ContextStateless{
func NewBuilderFromScratch(ioCtrl *io.Controller) *Builder {
builder := buildCmd.NewOktetoBuilder(
&okteto.ContextStateless{
Store: okteto.GetContextStore(),
},
Fs: afero.NewOsFs(),
}
registry := registry.NewOktetoRegistry(okteto.Config{})
return &OktetoBuilder{
Builder: builder,
Registry: registry,
ioCtrl: ioCtrl,
afero.NewOsFs(),
)

return &Builder{
BuildRunner: builder,
IoCtrl: ioCtrl,
}
}

// Build builds the images defined by a Dockerfile
func (bc *OktetoBuilder) Build(ctx context.Context, options *types.BuildOptions) error {
// Build builds the image defined by the BuildOptions used the BuildRunner passed as dependency
// of the builder
func (ob *Builder) Build(ctx context.Context, options *types.BuildOptions) error {
path := "."
if options.Path != "" {
path = options.Path
Expand All @@ -91,15 +82,31 @@ func (bc *OktetoBuilder) Build(ctx context.Context, options *types.BuildOptions)
return fmt.Errorf("%s: '%s' is not a regular file", oktetoErrors.InvalidDockerfile, options.File)
}

if err := bc.Builder.Run(ctx, options, bc.ioCtrl); err != nil {
var err error
options.Tag, err = env.ExpandEnv(options.Tag)
if err != nil {
return err
}

if err := ob.BuildRunner.Run(ctx, options, ob.IoCtrl); err != nil {
analytics.TrackBuild(false)
return err
}

if options.Tag == "" {
ob.IoCtrl.Out().Success("Build succeeded")
ob.IoCtrl.Out().Infof("Your image won't be pushed. To push your image specify the flag '-t'.")
} else {
tags := strings.Split(options.Tag, ",")
for _, tag := range tags {
displayTag := tag
if options.DevTag != "" {
displayTag = options.DevTag
}
ob.IoCtrl.Out().Success("Image '%s' successfully pushed", displayTag)
}
}

analytics.TrackBuild(true)
return nil
}

func (bc *OktetoBuilder) IsV1() bool {
return true
}
210 changes: 210 additions & 0 deletions cmd/build/basic/build_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
// Copyright 2024 The Okteto Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package basic

import (
"context"
"os"
"path/filepath"
"testing"

"github.com/okteto/okteto/pkg/env"
"github.com/okteto/okteto/pkg/log/io"
"github.com/okteto/okteto/pkg/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

type fakeBuildRunner struct {
mock.Mock
}

func (f *fakeBuildRunner) Run(ctx context.Context, buildOptions *types.BuildOptions, ioCtrl *io.Controller) error {
args := f.Called(ctx, buildOptions, ioCtrl)
return args.Error(0)
}

func TestBuildWithErrorFromDockerfile(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

tag := "okteto.dev/test"
options := &types.BuildOptions{
CommandArgs: []string{dir},
Tag: tag,
}

expectedOptions := &types.BuildOptions{
Path: dir,
File: filepath.Join(dir, "Dockerfile"),
Tag: tag,
CommandArgs: []string{dir},
}
buildRunner.On("Run", mock.Anything, expectedOptions, mock.Anything).Return(assert.AnError)

err = bc.Build(ctx, options)

// error from the build
assert.Error(t, err)
buildRunner.AssertExpectations(t)
}

func TestBuildWithNoErrorFromDockerfile(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

tag := "okteto.dev/test"
options := &types.BuildOptions{
CommandArgs: []string{dir},
Tag: tag,
}

expectedOptions := &types.BuildOptions{
Path: dir,
File: filepath.Join(dir, "Dockerfile"),
Tag: tag,
CommandArgs: []string{dir},
}
buildRunner.On("Run", mock.Anything, expectedOptions, mock.Anything).Return(nil)

err = bc.Build(ctx, options)
// no error from the build
assert.NoError(t, err)

buildRunner.AssertExpectations(t)
}

func TestBuildWithNoErrorFromDockerfileAndNoTag(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

options := &types.BuildOptions{
CommandArgs: []string{dir},
}

expectedOptions := &types.BuildOptions{
Path: dir,
File: filepath.Join(dir, "Dockerfile"),
CommandArgs: []string{dir},
}
buildRunner.On("Run", mock.Anything, expectedOptions, mock.Anything).Return(nil)

err = bc.Build(ctx, options)
// no error from the build
assert.NoError(t, err)

buildRunner.AssertExpectations(t)
}

func TestBuildWithErrorWithPathToFile(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

options := &types.BuildOptions{
Path: filepath.Join(dir, "Dockerfile"),
}

err = bc.Build(ctx, options)
// expected error from the build
assert.Error(t, err)

buildRunner.AssertNotCalled(t, "Run", mock.Anything, mock.Anything, mock.Anything)
}

func TestBuildWithErrorWithFileToDir(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

options := &types.BuildOptions{
CommandArgs: []string{dir},
Path: dir,
File: dir,
}

err = bc.Build(ctx, options)
// expected error from the build
assert.Error(t, err)

buildRunner.AssertNotCalled(t, "Run", mock.Anything, mock.Anything, mock.Anything)
}

func TestBuildWithErrorFromImageExpansion(t *testing.T) {
ctx := context.Background()

buildRunner := &fakeBuildRunner{}
bc := &Builder{
BuildRunner: buildRunner,
IoCtrl: io.NewIOController(),
}
dir, err := createDockerfile(t)
assert.NoError(t, err)

t.Setenv("TEST_VAR", "unit-test")
// The missing closing brace breaks the var expansion
tag := "okteto.dev/test:${TEST_VAR"
options := &types.BuildOptions{
CommandArgs: []string{dir},
Tag: tag,
}
err = bc.Build(ctx, options)
// error from the build
assert.ErrorAs(t, err, &env.VarExpansionErr{})

buildRunner.AssertNotCalled(t, "Run", mock.Anything, mock.Anything, mock.Anything)
}

func createDockerfile(t *testing.T) (string, error) {
dir := t.TempDir()
dockerfilePath := filepath.Join(dir, "Dockerfile")
err := os.WriteFile(dockerfilePath, []byte("Hello"), 0600)
if err != nil {
return "", err
}
return dir, nil
}
16 changes: 9 additions & 7 deletions cmd/build/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,8 @@ type registryInterface interface {
func NewBuildCommand(ioCtrl *io.Controller, analyticsTracker analyticsTrackerInterface, okCtx *okteto.ContextStateless, k8slogger *io.K8sLogger) *Command {

return &Command{
GetManifest: model.GetManifestV2,
Builder: &buildCmd.OktetoBuilder{
OktetoContext: okCtx,
Fs: afero.NewOsFs(),
},
GetManifest: model.GetManifestV2,
Builder: buildCmd.NewOktetoBuilder(okCtx, afero.NewOsFs()),
Registry: registry.NewOktetoRegistry(buildCmd.GetRegistryConfigFromOktetoConfig(okCtx)),
ioCtrl: ioCtrl,
k8slogger: k8slogger,
Expand Down Expand Up @@ -150,6 +147,10 @@ func Build(ctx context.Context, ioCtrl *io.Controller, at analyticsTrackerInterf
return cmd
}

// getBuilder returns the proper builder (V1 or V2) based on the manifest. The following rules are applied:
// - If the manifest is not found or there is any error getting the manifest, the builder fallsback to V1
// - If the manifest is found and it is a V2 manifest and the build section has some image, the builder is V2
// - If the manifest is found and it is a V1 manifest or the build section is empty, the builder fallsback to V1
func (bc *Command) getBuilder(options *types.BuildOptions, okCtx *okteto.ContextStateless) (Builder, error) {
var builder Builder

Expand All @@ -162,12 +163,12 @@ func (bc *Command) getBuilder(options *types.BuildOptions, okCtx *okteto.Context
bc.ioCtrl.Logger().Infof("manifest located at %s is not v2 compatible: %s", options.File, err)
bc.ioCtrl.Logger().Info("falling back to building as a v1 manifest")

builder = buildv1.NewBuilder(bc.Builder, bc.Registry, bc.ioCtrl)
builder = buildv1.NewBuilder(bc.Builder, bc.ioCtrl)
} else {
if isBuildV2(manifest) {
builder = buildv2.NewBuilder(bc.Builder, bc.Registry, bc.ioCtrl, bc.analyticsTracker, okCtx, bc.k8slogger)
} else {
builder = buildv1.NewBuilder(bc.Builder, bc.Registry, bc.ioCtrl)
builder = buildv1.NewBuilder(bc.Builder, bc.ioCtrl)
}
}

Expand All @@ -177,6 +178,7 @@ func (bc *Command) getBuilder(options *types.BuildOptions, okCtx *okteto.Context
}

func isBuildV2(m *model.Manifest) bool {
// A manifest has the isV2 set to true if the manifest is parsed as a V2 manifest or in case of stacks and/or compose files
return m.IsV2 && len(m.Build) != 0
}

Expand Down

0 comments on commit fb49fc0

Please sign in to comment.