Partial implementation of artifacts for test command

[maroshii]

Proposed changes

Adds artifacts to test command that allows to export files to the local file system after buildkit execution:

test:
  echo:
    artifacts:
      - a
      - b
      - c
      - d
    commands:
      - mkdir a b c d
      - echo "A" > a/1.txt
      - echo "B" > b/2.txt
      - echo "C" > c/3.txt
      - echo "D" > d/4.txt
      - ls -l

The above manifest will export the following the local filesystem:

./.okteto-output/
├── a
│   └── 1.txt
├── b
│   └── 2.txt
├── c
│   └── 3.txt
└── d
    └── 4.txt

Open Questions

• Do we want to write to .okteto-output or do we rather just right relative to the cwd?

---

⚠️ CAVEAT ⚠️

This was marked as a partial implementation because it only works for successful builds. If the build fails nothing will get written. I'm not sure if buildkit has a way to export regardless of failure but this is still a big unknown for this feature. The docs for local export has no info on this and assumes that the build succeeds. I'll keep investigating cc @pchico83

[andreafalzetti]

Code changes LGTM. About the failing build I have no suggestions yet, but I've done some initial tests, so far so good, only one odd behaviour perhaps with the following scenario, I think due to cache, when exporting artifacts, it also includes previous ones. For example:

test:
  unit:
    context: unit
    artifacts:
      - /
    commands:
      - name: Run unit tests
        command: mkdir report && echo hola > report/hola.txt && echo ciao > report/ciao.txt

okteto test

# i  Using ******@ ***** as context
# i  'go-getting-started' was already deployed. To redeploy run 'okteto deploy'
# i  Executing 'unit'


ls -la .okteto-output
drwx------   4 andrea  staff  128 20 May 22:31 .
drwxr-xr-x  19 andrea  staff  608 20 May 22:31 ..
-rw-r--r--   1 andrea  staff    5 20 May 09:26 hola.txt
drwxr-xr-x   4 andrea  staff  128 20 May 22:28 report

ls -la .okteto-output/report
drwxr-xr-x  4 andrea  staff  128 20 May 22:28 .
drwx------  4 andrea  staff  128 20 May 22:31 ..
-rw-r--r--  1 andrea  staff    5 20 May 22:28 ciao.txt
-rw-r--r--  1 andrea  staff    5 20 May 22:28 hola.txt

then modify okteto.yml and change the manifest to:

test:
  unit:
    context: unit
    artifacts:
      - /
    commands:
      - name: Run unit tests
        command: mkdir report && echo hola > report/hola2.txt && echo ciao > report/ciao2.txt

Run again and observe the duplicates:

okteto test

# i  Using ******@ ***** as context
# i  'go-getting-started' was already deployed. To redeploy run 'okteto deploy'
# i  Executing 'unit'


ls -la .okteto-output
drwx------   4 andrea  staff  128 20 May 22:38 .
drwxr-xr-x  19 andrea  staff  608 20 May 22:37 ..
-rw-r--r--   1 andrea  staff    5 20 May 09:26 hola.txt
drwxr-xr-x   6 andrea  staff  192 20 May 22:38 report

ls -la .okteto-output/report
drwxr-xr-x  6 andrea  staff  192 20 May 22:38 .
drwx------  4 andrea  staff  128 20 May 22:38 ..
-rw-r--r--  1 andrea  staff    5 20 May 22:28 ciao.txt
-rw-r--r--  1 andrea  staff    5 20 May 22:38 ciao2.txt
-rw-r--r--  1 andrea  staff    5 20 May 22:28 hola.txt
-rw-r--r--  1 andrea  staff    5 20 May 22:38 hola2.txt

If I use this to export test reports, especially for HTML based reports, it's very likely that this could be an issue. WDYT?

[pchico83]

I think exporting only on success is okay for this iteration. I don't think buildkit will help, we would need to always return success at the buildkit level and manage the error on our side.
About .okteto-output, I think the experience is better if we write relative to the cwd. That helps, for example, if you do things like go mod tidy. In general, adopting the Okteto Test requires fewer changes if we use the current path. And we can always make this path optional if it becomes an issue

[jLopezbarb]

Should we support something like what CircleCI does to export it to a custom local path:

- store_artifacts:
          path: /tmp/artifact-1
          destination: artifact-file

I think it's ok to have a default destination but I think the user could want to export to a specific file that is already in the .oktetoignore. WDYT?

[jLopezbarb]

Tested and working correctly. Also tested removing a file and checked if the cache would regenerate the file. So despite of the question that I asked I think the code and behaviour is ok

[maroshii]

Should we support something like what CircleCI does to export it to a custom local path:

- store_artifacts:
          path: /tmp/artifact-1
          destination: artifact-file

I think it's ok to have a default destination but I think the user could want to export to a specific file that is already in the .oktetoignore. WDYT?

If we want to have buildkit and the concept of containerization abstracted away from the user, path/destination doesn't really mean anything. I think it makes sense if we want to expose (conceptually) that the execution context is not the user's filesystem. Given that we already have "context" in the test manifest and that we do document how tests and remote deploy works, we are already in the path of not hiding it away so I would add it 👌

[maroshii]

I don't think buildkit will help, we would need to always return success at the buildkit level and manage the error on our side.

Yeah, agree that's the way to go. It will have an impact on buildkit metrics tho since once we change this all builds will succeed

[maroshii]

If I use this to export test reports, especially for HTML based reports, it's very likely that this could be an issue. WDYT?

@andreafalzetti Looking at it in more detail, I think that's the expected behavior. If you add an && ls -l in the command you will see that the previous files are not in the container filesystem which is good.

I think the "issue" is that when files are exported, buildkit is smart enough to merge the local filesystem with the exported files. I think it's acceptable, if user don't want this they can nuke the folder before running okteto test. In CI it will have no effect I guess

[maroshii]

PR is good to go. New changes:

• Extended syntax for path and destination in artifact section is supported
• output is no longer written to ./okteto-output, it is now written to the context root path
• fix linting errors

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 43.11%. Comparing base (98675e6) to head (1bac634).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4302      +/-   ##
==========================================
+ Coverage   43.07%   43.11%   +0.04%     
==========================================
  Files         370      371       +1     
  Lines       30020    30045      +25     
==========================================
+ Hits        12931    12954      +23     
+ Misses      15966    15964       -2     
- Partials     1123     1127       +4     

[pchico83]

I don't think buildkit will help, we would need to always return success at the buildkit level and manage the error on our side.

Yeah, agree that's the way to go. It will have an impact on buildkit metrics tho since once we change this all builds will succeed

@maroshii we don't rely on buildkit for metrics, the build status is reported by the Okteto CLI. We would also have control on the metrics generated by failed builds

[pchico83]

• Extended syntax for path and destination in artifact section is supported

@maroshii could you explain the new syntax with a few samples if needed?

[andreafalzetti]

as discussed offline, with the latest changes in this PR, the logs of the test commands are not visibile. Example

test:
  load:
    context: .
    commands:
      - name: "Say Hi"
        command: echo hiii && exit 1

With 2.27.21 I get:

 i  Using ** @ ** as context
 i  'tmp.RMs0oBYF6M' was already deployed. To redeploy run 'okteto deploy'
 i  Executing 'load'
 i  Running stage 'load'
 i  Running 'Say Hi'
hiii
 x  Error on stage load:  x  Exit status 1

With this build:

 i  Using ** @ ** as context
 i  'tmp.RMs0oBYF6M' was already deployed. To redeploy run 'okteto deploy'
 i  Executing 'load'

[maroshii]

could you explain the new syntax with a few samples if needed?

@pchico83 will do. We still need to update the docs with everything new we added besides this and the latest changes. PRs are in place but postponing until we are feature complete

[maroshii]

as discussed offline, with the latest changes in this PR, the logs of the test commands are not visibile. Example

@andreafalzetti thanks for report this. It was indeed a regression.

The error was that when no artifacts were defined, there were no CPOY --from=runner ... instructions in the final image so the runner image was never built, just the scratch. This is because BuildKit only builds the stages that the target stage depends on as opposed to the previous docker builder

Now, we save the cachekey value in a file and include it no the final image if no artifacts are defined, which ensures the runner image is always considered in builds.

Fixed here: a98bce5