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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix panic when inferring stack with empty image #4312

Merged
merged 2 commits into from
May 27, 2024

Conversation

andreafalzetti
Copy link
Contributor

@andreafalzetti andreafalzetti commented May 23, 2024

Proposed changes

Fixes DEV-368

This PR fixes a panic that is caused when using Docker Compose and Okteto Manifests together and the Image field of the service is empty. For example, when running okteto down or okteto destroy the image is not built, which may cause the image field to be empty, in case the compose rely on built-in Okteto Variables such as ${OKTETO_BUILD_<SERVICE>_IMAGE}

This is a panic that was introduced in 2.27.x because of recent changes.

How to validate

  1. Using https://github.com/okteto/movies-with-compose
  2. Change services.app.image to ${OKTETO_BUILD_APP_IMAGE}
  3. Create the following okteto.yml
build:
  app:
    context: api
deploy:
  compose: docker-compose.yml
  1. Run okteto build and observe the panic
  2. Now repeat with the new build and validate that the panic does not occurr
  3. Now test that deploy + up + down and destroy work as expected and cause no panics.

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

@andreafalzetti andreafalzetti added release/bug-fix run-e2e When used on a PR run windows & unix e2e backport release-2.27 Backport this PR to CLI version 2.27 labels May 23, 2024
@jLopezbarb jLopezbarb mentioned this pull request May 23, 2024
@andreafalzetti andreafalzetti force-pushed the af/dev-368-panic-compose branch 4 times, most recently from ac03fb0 to edd9899 Compare May 23, 2024 12:15
@@ -35,7 +35,7 @@ import (
oktetoLog "github.com/okteto/okteto/pkg/log"
"github.com/okteto/okteto/pkg/model/forward"
"github.com/spf13/afero"
yaml "gopkg.in/yaml.v2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

goland insists in removing this, which is redundant, happy to revert it if we prefer so.

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Copy link

codecov bot commented May 25, 2024

Codecov Report

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

Project coverage is 43.09%. Comparing base (19eb936) to head (488a3c3).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4312      +/-   ##
==========================================
- Coverage   43.10%   43.09%   -0.01%     
==========================================
  Files         371      371              
  Lines       30021    30025       +4     
==========================================
  Hits        12940    12940              
- Misses      15954    15956       +2     
- Partials     1127     1129       +2     

@andreafalzetti andreafalzetti changed the title WIP: fix panic when inferring stack with empty image fix panic when inferring stack with empty image May 25, 2024
@andreafalzetti andreafalzetti marked this pull request as ready for review May 25, 2024 12:10
@andreafalzetti andreafalzetti requested a review from a team as a code owner May 25, 2024 12:10
@andreafalzetti andreafalzetti added the backport release-2.26 Backport this PR to CLI version 2.26 label May 27, 2024
@andreafalzetti andreafalzetti merged commit 260bfeb into master May 27, 2024
18 of 19 checks passed
@andreafalzetti andreafalzetti deleted the af/dev-368-panic-compose branch May 27, 2024 13:54
github-actions bot pushed a commit that referenced this pull request May 27, 2024
* fix panic when inferring stack with empty image

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

* add e2e test

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

---------

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
(cherry picked from commit 260bfeb)
github-actions bot pushed a commit that referenced this pull request May 27, 2024
* fix panic when inferring stack with empty image

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

* add e2e test

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

---------

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
(cherry picked from commit 260bfeb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.26 Backport this PR to CLI version 2.26 backport release-2.27 Backport this PR to CLI version 2.27 release/bug-fix run-e2e When used on a PR run windows & unix e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants