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: clean manifest path in remote deploy to fix regression #4195

Merged
merged 4 commits into from Mar 8, 2024

Conversation

andreafalzetti
Copy link
Contributor

@andreafalzetti andreafalzetti commented Mar 5, 2024

Proposed changes

Fixes DEV-199

How to validate

  1. Clone my test repo on the given branch: git clone -b fix-dev-199 git@github.com:andreafalzetti/k8s-status-api.git
  2. Using the 2.25.1 reproduce the bug by running: okteto up --deploy -f apps/service/okteto.yml
  3. Observe the error: Okteto.yml file doesn't exist
  4. Now run the same command with the CLI from this branch
  5. Notice how the okteto up session, works as expected

Repeat the steps for destroy and make sure it works as expected

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 labels Mar 5, 2024
@andreafalzetti andreafalzetti requested a review from a team as a code owner March 5, 2024 13:16
@andreafalzetti andreafalzetti marked this pull request as draft March 5, 2024 13:18
@andreafalzetti andreafalzetti added the backport release-2.25 Backport this PR to CLI version 2.25 label Mar 5, 2024
@andreafalzetti andreafalzetti marked this pull request as ready for review March 5, 2024 14:16
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Merging #4195 (a88af29) into master (fb49fc0) will decrease coverage by 0.04%.
The diff coverage is 83.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4195      +/-   ##
==========================================
- Coverage   45.71%   45.68%   -0.04%     
==========================================
  Files         291      292       +1     
  Lines       27198    27194       -4     
==========================================
- Hits        12434    12424      -10     
- Misses      13676    13680       +4     
- Partials     1088     1090       +2     

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

@ifbyol ifbyol left a comment

Choose a reason for hiding this comment

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

We need to have that logic also in the destroy. Check my inline comment

@@ -327,6 +327,19 @@ func (rd *remoteDeployCommand) createDockerfile(tmpDir string, opts *Options) (s
return dockerfile.Name(), nil
}

// cleanManifestPath removes the path to the manifest file, in case the command was executed from a parent or child folder
func cleanManifestPath(manifestPath string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this same logic in the destroy? If the destroy has some commands to be executed, it won't execute them, but the dev-environment will be deleted.

I have tested it adding this to your example:

destroy:
  remote: true
  commands:
    - echo "Hello world"

If you execute okteto destroy -f apps/service/okteto.yml, it doesn't run the echo command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ifbyol good point, I've made the changes for destroy and moved the cleanManifestPath to a shared place. Can you please have another go at the review?

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
@andreafalzetti andreafalzetti merged commit e568f2a into master Mar 8, 2024
14 checks passed
@andreafalzetti andreafalzetti deleted the af/fix-remote-deploy-dev-199 branch March 8, 2024 08:15
Copy link
Contributor

github-actions bot commented Mar 8, 2024

The backport to release-2.25 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-2.25 release-2.25
# Navigate to the new working tree
cd .worktrees/backport-release-2.25
# Create a new branch
git switch --create backport-4195-to-release-2.25
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e568f2a875384e705af5adec861b667f048f8706
# Push it to GitHub
git push --set-upstream origin backport-4195-to-release-2.25
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-2.25

Then, create a pull request where the base branch is release-2.25 and the compare/head branch is backport-4195-to-release-2.25.

andreafalzetti added a commit that referenced this pull request Mar 14, 2024
---------

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
andreafalzetti added a commit that referenced this pull request Mar 18, 2024
* backport remote build context

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

* fix: clean manifest path in remote deploy to fix regression (#4195)

---------

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

* fix: getContextPath when manifestpath is subdir

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

* clean should not return dot

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

* fix: .okteto

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

* fix: 2nd scenario

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

* refactor: simplify and remove unnecessary code

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

* fix: additional case

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

---------

Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.25 Backport this PR to CLI version 2.25 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