fix: clone images on specified services #4270

jLopezbarb · 2024-04-29T16:56:23Z

Proposed changes

Fix multi tags with okteto registries.
- When we run a command like okteto build -t okteto.dev/myapp:1.0.0,okteto.dev/myapp:latest
- It was preventing us to create a clone
Create a new clone function that clones one image into another without calculating anything
- Reuse that new function in the cloneFromGlobalToDev
Fix: Smartbuild was not getting the correct interface
- The interface was the same but the methods were named different. In this PR we use the same naming
Created getGlobalTagFromDevIfNeccesary in order to get the global tag from dev if we need to build into the global registry too

It has created a side effect where we show that the image has been successfully pushed into the two registries. I think it's better for the user so he knows what's happening. Are we ok with that

How to validate

Copy the following okteto.ylm

build:
  my-service:
    image: okteto.dev/my-service:my-tag
    context: my-service

Create my-service folder with a simple Dockerfile (FROM alpine)
Run okteto build
Check that we are pushing to the dev registry to the image in the field and the global registry with the hash as the tag

CLI Quality Reminders 🔧

For both authors and reviewers:

Scrutinize for potential regressions
Ensure key automated tests are in place
Build the CLI and test using the validation steps
Assess Developer Experience impact (log messages, performances, etc)
If too broad, consider breaking into smaller PRs
Adhere to our code style and code review guidelines

Signed-off-by: Javier Lopez <[email protected]>

jLopezbarb · 2024-04-29T16:57:28Z

cmd/build/v2/tagger.go

@@ -92,6 +94,19 @@ func (it imageTagger) getServiceImageReference(manifestName, svcName string, b *
 return useReferenceTemplate(targetRegistry, sanitizedName, svcName, model.OktetoDefaultImageTag)
 }

+func (it imageTagger) getGlobalTagFromDevIfNeccesary(tags, namespace, registryURL, buildHash, manifestName, svcName string, ic registry.ImageCtrl) string {
+ tagList := strings.Split(tags, ",")


We need to expand this to build all the global images from the parsed tags

jLopezbarb · 2024-04-29T16:57:52Z

pkg/cmd/build/build.go

- E: fmt.Errorf("'%s' isn't a valid image tag", imageTag),
- Hint: fmt.Sprintf("The Okteto Registry syntax is: '%s/image_name'", prefix),
+ tags := strings.Split(imageTags, ",")
+ for _, tag := range tags {


Fix to pass several tags with okteto.dev

Signed-off-by: Javier Lopez <[email protected]>

codecov · 2024-05-21T17:10:47Z

Codecov Report

Attention: Patch coverage is 76.72414% with 27 lines in your changes missing coverage. Please review.

Project coverage is 43.25%. Comparing base (eb683fb) to head (634c93e).
Report is 5 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4270   +/-   ##
=======================================
  Coverage   43.25%   43.25%           
=======================================
  Files         375      375           
  Lines       30200    30222   +22     
=======================================
+ Hits        13064    13074   +10     
- Misses      15989    16007   +18     
+ Partials     1147     1141    -6

Signed-off-by: Javier Lopez <[email protected]>

ifbyol · 2024-05-24T07:32:57Z

It has created a side effect where we show that the image has been successfully pushed into the two registries. I think it's better for the user so he knows what's happening. Are we ok with that

@jLopezbarb Why the double message? Before it was also happening but only on BuildKit logs, but not in the final success step

ifbyol

As we talked offline, we are mixing 2 ways of cloning images:

Old one: If the user didn't specify an image in the build info, we pass one tag in buildInfo.Tag and then, in buildkit.go, we fill buildOptions.DevTag with the dev tag to push
New one (added in this PR). If the user specify an image, and it is for dev registry, we pass 2 images in buildOptions.Tag and then, DevTag is not filled just because dev tag is the first in the list

We should unify both ways to just one to avoid problems in the future

ifbyol · 2024-05-24T08:12:14Z

cmd/build/v2/build.go

+ if ob.Registry.IsOktetoRegistry(firstTag) {
+ imageWithDigest, err = ob.smartBuildCtrl.Clone(imageWithDigest, firstTag)
+ if err != nil {
+ return err
+ }
+ }
+ } else {
+ imageWithDigest, err = ob.smartBuildCtrl.CloneGlobalImageToDev(imageWithDigest)
+ if err != nil {


Can't we unify this somehow instead of having to have 2 different functions (very similar)? At the end, CloneGlobalImageToDev wil have to clone always to okteto.dev/<image>:okteto. WDYT? Would it make sense?

pkg/cmd/build/build.go

cmd/build/v2/build.go

Signed-off-by: Javier Lopez <[email protected]>

jLopezbarb · 2024-05-29T09:33:39Z

As we talked offline, we are mixing 2 ways of cloning images:

Old one: If the user didn't specify an image in the build info, we pass one tag in buildInfo.Tag and then, in buildkit.go, we fill buildOptions.DevTag with the dev tag to push

New one (added in this PR). If the user specify an image, and it is for dev registry, we pass 2 images in buildOptions.Tag and then, DevTag is not filled just because dev tag is the first in the list

We should unify both ways to just one to avoid problems in the future

On the old one we were only adding the dev tag if the image was a global image. I've removed the code so we are only using the new method

Signed-off-by: Javier Lopez <[email protected]>

cmd/build/v2/build.go

Signed-off-by: Javier Lopez <[email protected]>

cmd/build/v2/smartbuild/smartbuild.go

cmd/build/v2/tagger.go

Signed-off-by: Javier Lopez <[email protected]>

ifbyol · 2024-06-10T10:50:12Z

cmd/build/v2/smartbuild/smartbuild_test.go

@@ -185,70 +188,6 @@ func TestGetBuildHash(t *testing.T) {
 assert.Equal(t, "hash", out)
 }

-func TestCloneGlobalImageToDev(t *testing.T) {


I think you need to change also the tests. It is no longer TestClone

ifbyol · 2024-06-10T15:08:04Z

Discussed offline, there is 1 case that doesn't work as expected. If I set my own image with a specific name, it is being cloned to global with other name instead of using the 1 I specified

Signed-off-by: Javier Lopez <[email protected]>

teresaromero

Tested couple scenarios, LGTM

Signed-off-by: Javier Lopez <[email protected]>

ifbyol · 2024-06-11T15:31:02Z

I found another issue while validating the changes. It is related to my previous message: #4270 (comment)

When I specify a custom image in the image field pointing to an Okteto dev registry image, e.g. okteto.dev/<image-name>:<my-custom-tag>, Smart Builds is not checking the right image. Instead of using the image name specified in image field, it is calculating the image name based on the service name. It should also check using the name specified in the image field

Signed-off-by: Javier Lopez <[email protected]>

fix: clone images on specified services

c708ddc

Signed-off-by: Javier Lopez <[email protected]>

jLopezbarb commented Apr 29, 2024

View reviewed changes

jLopezbarb added the release/bug-fix label May 10, 2024

jLopezbarb added 4 commits May 21, 2024 17:45

fix: golang ci

6a04324

Signed-off-by: Javier Lopez <[email protected]>

Merge branch 'master' into jlo/dev-232

41dd75c

Signed-off-by: Javier Lopez <[email protected]>

fix: unit tests

2b93e07

Signed-off-by: Javier Lopez <[email protected]>

fix: golangci

df6157d

Signed-off-by: Javier Lopez <[email protected]>

feat: add unit tests

ae42984

Signed-off-by: Javier Lopez <[email protected]>

jLopezbarb marked this pull request as ready for review May 22, 2024 10:51

jLopezbarb requested a review from a team as a code owner May 22, 2024 10:51

fix: golangci

a7a202e

Signed-off-by: Javier Lopez <[email protected]>

ifbyol reviewed May 24, 2024

View reviewed changes

fix: remove devTag

64f3450

Signed-off-by: Javier Lopez <[email protected]>

fix: clone as one way

8a06245

Signed-off-by: Javier Lopez <[email protected]>

ifbyol reviewed May 30, 2024

View reviewed changes

cmd/build/v2/build.go Outdated Show resolved Hide resolved

cmd/build/v2/build.go Outdated Show resolved Hide resolved

jLopezbarb requested a review from ifbyol May 30, 2024 11:04

jLopezbarb added 3 commits May 30, 2024 14:16

address feedback

9d08c6e

Signed-off-by: Javier Lopez <[email protected]>

address feedback

e48fc14

Signed-off-by: Javier Lopez <[email protected]>

Merge branch 'master' into jlo/dev-232

6ffce57

Signed-off-by: Javier Lopez <[email protected]>

ifbyol reviewed May 31, 2024

View reviewed changes

cmd/build/v2/smartbuild/smartbuild.go Outdated Show resolved Hide resolved

cmd/build/v2/tagger.go Outdated Show resolved Hide resolved

cmd/build/v2/tagger.go Outdated Show resolved Hide resolved

jLopezbarb added 3 commits June 4, 2024 13:13

address feedback

99ab5ae

Signed-off-by: Javier Lopez <[email protected]>

revert to cloneGlobalImageToDev

940d57c

Signed-off-by: Javier Lopez <[email protected]>

fix: golangci

750c70c

Signed-off-by: Javier Lopez <[email protected]>

ifbyol reviewed Jun 10, 2024

View reviewed changes

fix: use case and add tests

41a5fa1

Signed-off-by: Javier Lopez <[email protected]>

teresaromero approved these changes Jun 11, 2024

View reviewed changes

fix: linter

97dd668

Signed-off-by: Javier Lopez <[email protected]>

fix: scenario

634c93e

Signed-off-by: Javier Lopez <[email protected]>

ifbyol approved these changes Jun 13, 2024

View reviewed changes

jLopezbarb merged commit 7870e0e into master Jun 14, 2024
11 of 12 checks passed

jLopezbarb deleted the jlo/dev-232 branch June 14, 2024 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: clone images on specified services #4270

fix: clone images on specified services #4270

jLopezbarb commented Apr 29, 2024 •

edited

jLopezbarb Apr 29, 2024

jLopezbarb Apr 29, 2024

codecov bot commented May 21, 2024 •

edited

ifbyol commented May 24, 2024

ifbyol left a comment

ifbyol May 24, 2024

jLopezbarb commented May 29, 2024

ifbyol Jun 10, 2024

ifbyol commented Jun 10, 2024

teresaromero left a comment

ifbyol commented Jun 11, 2024

fix: clone images on specified services #4270

fix: clone images on specified services #4270

Conversation

jLopezbarb commented Apr 29, 2024 • edited

Proposed changes

How to validate

CLI Quality Reminders 🔧

jLopezbarb Apr 29, 2024

Choose a reason for hiding this comment

jLopezbarb Apr 29, 2024

Choose a reason for hiding this comment

codecov bot commented May 21, 2024 • edited

Codecov Report

ifbyol commented May 24, 2024

ifbyol left a comment

Choose a reason for hiding this comment

ifbyol May 24, 2024

Choose a reason for hiding this comment

jLopezbarb commented May 29, 2024

ifbyol Jun 10, 2024

Choose a reason for hiding this comment

ifbyol commented Jun 10, 2024

teresaromero left a comment

Choose a reason for hiding this comment

ifbyol commented Jun 11, 2024

jLopezbarb commented Apr 29, 2024 •

edited

codecov bot commented May 21, 2024 •

edited