fix: use specific terminal #3019

jLopezbarb · 2022-08-19T10:59:48Z

Signed-off-by: Javier López Barba javier@okteto.com

Proposed changes

Use the same terminal as the one executing the main okteto command

Concerns

Since this is running remote and local I want to make sure it doesn't have any other implications.

I have been testing in Windows on different terminals and it returns the correct user and we don't have the problem of absolute paths anymore.

I have also tested on Linux machines and there seems to be no problem, but as this is a change that affects one of the main okteto commands I would like to know your opinion regarding this change.

Signed-off-by: Javier López Barba <javier@okteto.com>

codecov · 2022-08-19T11:02:15Z

Codecov Report

Merging #3019 (a495f5a) into master (09fd1d6) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3019      +/-   ##
==========================================
- Coverage   32.73%   32.73%   -0.01%     
==========================================
  Files         188      188              
  Lines       19748    19750       +2     
==========================================
  Hits         6465     6465              
- Misses      12518    12520       +2     
  Partials      765      765

Impacted Files	Coverage Δ
cmd/deploy/deploy.go	`27.24% <0.00%> (-0.08%)`	⬇️
cmd/destroy/destroy.go	`42.05% <0.00%> (-0.20%)`	⬇️
cmd/manifest/init-v2.go	`0.00% <0.00%> (ø)`
cmd/up/up.go	`10.71% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AgustinRamiroDiaz · 2022-08-19T11:54:25Z

I guess this is how it should've been from the start, but I'm also concerned if the wrapping around bash -c could have been due to some case we wanted to handle. Do the e2e tests run in different OS? That's something that will add more certainty

Also, I don't know if someone was using this as a kind of a "feature". i.e. using another terminal like fish and then putting bash commands in their okteto.yml to be compatible with the rest of their team. I think this is a rare case though and I'm in favor of the consistency of running the commands in the corresponding terminal.

jLopezbarb · 2022-08-19T12:28:59Z

I guess this is how it should've been from the start, but I'm also concerned if the wrapping around bash -c could have been due to some case we wanted to handle. Do the e2e tests run in different OS? That's something that will add more certainty

The e2e tests run on both Windows and Linux, the tests will catch some errors, but because the cases are common (deploy with helm, kubectl, or stack) I don't think we can get any errors from them.

Also, I don't know if someone was using this as a "feature". i.e. using another terminal like fish and then putting bash commands in their okteto.yml to be compatible with the rest of their team. I think this is a rare case though and I'm in favor of the consistency of running the commands in the corresponding terminal.

I don't think people used it as a feature since it changed both the OS and the working directory from which the command was launched.

rberrelleza · 2022-08-20T18:47:43Z

cmd/utils/executor/executor.go

@@ -61,7 +61,7 @@ func NewExecutor(output string) *Executor {
 // Execute executes the specified command adding `env` to the execution environment
 func (e *Executor) Execute(cmdInfo model.DeployCommand, env []string) error {

-	cmd := exec.Command("bash", "-c", cmdInfo.Command)
+	cmd := exec.Command(cmdInfo.Command)


This is going to change how commands are executed. Could we set this as a flag, announce deprecation, then do the full switch in a version or two? (Or announce it as a breaking change).

@rberrelleza what do you mean by setting this as a flag? the new behavior under the flag? If we do that, we'll be throwing a warning every time a user runs okteto deploy/destroy to use a file flag that is going to be deprecated.

If we set the old behavior under the flag, are we going to release a flag that is already deprecated? I prefer this idea and show the users a way of getting back the old behavior(via flag). We should also track how many people are having errors vs previous versions and see if we are having more errors after this change, WDYT?

Setting as default the new behavior could break customers' scripts/pipelines which ideally we should try to avoid unless we announce it as a breaking change and update the versions accordingly.

If I understand correctly one of the issues, does this prevent the okteto deploy command to work on Windows? If so, we might enforce the new behavior just when the OS is windows. I mean, if for those cases it's not working, we won't break anything changing the behavior. For the rest of OS we can include a flag defaulting to the old behavior

@ifbyol The problem is that windows bash runs on an auxiliary operating system, which does not run on the same path or with the same user. Being an auxiliary operating system it does not correctly pair absolute paths.

We can activate it only for Windows, although I would like that there is no difference between different operating systems, since it is more difficult to maintain it.

@jLopezbarb I know, but one of our main goals is stability in the CLI, so a breaking change I don't think is a good option right now.

My point is, if in Windows this was never working as expected, doing this breaking change only for Windows would fix the issue. Now, if this wasn't working properly on some situations, then, maybe we have to rethink it because we could be broken other scenarios

What if we add a feature flag to remove the bach -c? In this case we wouldn't do a breaking change and we'd also allow for the "correct" behavior with a flag like --execute-without-bash. We could incentivize the use of this flag and postpone it as the default behavior in some future big release

WDYT @ifbyol @jLopezbarb ?

Signed-off-by: Javier López Barba <javier@okteto.com>

cmd/utils/executor/executor.go

Signed-off-by: Javier López Barba <javier@okteto.com>

cmd/deploy/deploy.go

cmd/destroy/destroy.go

AgustinRamiroDiaz

I think this is a sweet spot between usability and backwards compatibility. We have to bear in mind we are adding another flag to the cli, which is something we are keeping an eye on since we are aiming for more stability

Signed-off-by: Javier López Barba <javier@okteto.com>

jLopezbarb · 2022-08-30T16:47:44Z

@RinkiyaKeDad @ralamana I don't like the fact of having a flag with a negative preposition --run-without-bash. Do you think that the flag can be renamed so that this negative term is not used?

rberrelleza · 2022-08-30T18:55:42Z

@RinkiyaKeDad @ralamana I don't like the fact of having a flag with a negative preposition --run-without-bash. Do you think that the flag can be renamed so that this negative term is not used?

Agree with Javi. This is a very typical problem when running commands, what do other tools or platforms do?

rberrelleza · 2022-08-30T18:58:11Z

I guess this is how it should've been from the start, but I'm also concerned if the wrapping around bash -c could have been due to some case we wanted to handle. Do the e2e tests run in different OS? That's something that will add more certainty

Also, I don't know if someone was using this as a kind of a "feature". i.e. using another terminal like fish and then putting bash commands in their okteto.yml to be compatible with the rest of their team. I think this is a rare case though and I'm in favor of the consistency of running the commands in the corresponding terminal.

This is a very good point. I don't recall why it was added, but we didn't have -c from day 1; it was added to fix error conditions (maybe with env var injection?). Has someone gone through the commit history to see if there are any clues?

AgustinRamiroDiaz · 2022-08-30T23:25:42Z

@RinkiyaKeDad @ralamana I don't like the fact of having a flag with a negative preposition --run-without-bash. Do you think that the flag can be renamed so that this negative term is not used?

What if we rename the flag --run-with-bash and set the default value to true with Cobra?

AgustinRamiroDiaz · 2022-08-30T23:27:30Z

@rberrelleza "Has someone gone through the commit history to see if there are any clues?" I've tried a bit and didn't find any clue. I used the vscode extension GitLens which helped find the PR, if anyone is interested in looking for something

RinkiyaKeDad · 2022-08-31T08:01:08Z

What if we rename the flag --run-with-bash and set the default value to true with Cobra?

+1 for this suggestion. We could also then simplify the name of the flag to just --bash maybe and make it clear in the help section what it does.

ifbyol · 2022-08-31T09:02:11Z

@rberrelleza @AgustinRamiroDiaz @jLopezbarb I think that the bash -c came when we created the okteto deploy command and we took the code from the backend installer. That code was there and we moved it here.

In the installer, we always had control on the terminal and platform where the command was being run and we didn't consider that when we moved the code to the CLI.

What if we rename the flag --run-with-bash and set the default value to true with Cobra?

That could work, but it would be also interesting to see how other CLI tools do it

jLopezbarb · 2022-08-31T09:23:21Z

What if we rename the flag --run-with-bash and set the default value to true with Cobra?

+1 for this suggestion. We could also then simplify the name of the flag to just --bash maybe and make it clear in the help section what it does.

I don't like too much the idea of --bash because the user will have to run okteto deploy --bash=false which is not intuitive on boolean flags. I prefer to use something like okteto deploy --use-same-shell although I don't like the name of that flag.

@rberrelleza "Has someone gone through the commit history to see if there are any clues?" I've tried a bit and didn't find any clue. I used the vscode extension GitLens which helped find the PR, if anyone is interested in looking for something

I can see that the bash -c has been there since the start of the pipeline installer. Although, in the tests that I made it did not give errors I do not think that we can remove it and we have to use a flag to change the behavior.

AgustinRamiroDiaz · 2022-08-31T10:49:20Z

@jLopezbarb @ifbyol regarding the naming, a common convention is to shorten the wording with expressions like --no-bash (like the classic --no-cache flag). It's really short, but I prefer the long, more explicit name since it's a behavior only a few users will need, so typing it out is not a problem

https://github.com/moby/buildkit/blob/9528036502c589626bfb60600af6ce5ee427f479/cmd/buildctl/build.go#L67

jLopezbarb · 2022-08-31T11:01:17Z

@jLopezbarb @ifbyol regarding the naming, a common convention is to shorten the wording with expressions like --no-bash (like the classic --no-cache flag). It's really short, but I prefer the long, more explicit name since it's a behavior only a few users will need, so typing it out is not a problem

https://github.com/moby/buildkit/blob/9528036502c589626bfb60600af6ce5ee427f479/cmd/buildctl/build.go#L67

+1 to --no-bash! Do we have documented that the commands are executed on a bash terminal? If we have that we can refer to the flag and the information about deploy executing from a bash terminal. If not I'll create it before releasing the new CLI version.
If the question/error keeps popping up, we could include a question on FAQ to guide the users.

AgustinRamiroDiaz · 2022-08-31T13:18:42Z

Do we have documented that the commands are executed on a bash terminal?

I couldn't find any. I think we should do both:

document that commands are executed wrapped around bash -c
document the new flag to disable that behavior

Signed-off-by: Javier López Barba <javier@okteto.com>

fix: use specific terminal

6200e7d

Signed-off-by: Javier López Barba <javier@okteto.com>

jLopezbarb requested a review from a team August 19, 2022 10:59

jLopezbarb linked an issue Aug 19, 2022 that may be closed by this pull request

okteto deploy doesn't accept absolute paths on windows #2820

Closed

jLopezbarb added the release/bug-fix label Aug 19, 2022

AgustinRamiroDiaz approved these changes Aug 19, 2022

View reviewed changes

rberrelleza approved these changes Aug 20, 2022

View reviewed changes

jLopezbarb added 2 commits August 24, 2022 12:49

fix: use function to describe the change

085408a

Signed-off-by: Javier López Barba <javier@okteto.com>

fix: unnit tests

ea19063

Signed-off-by: Javier López Barba <javier@okteto.com>

AdrianPedriza reviewed Aug 30, 2022

View reviewed changes

cmd/utils/executor/executor.go Outdated Show resolved Hide resolved

fix: address comments

b404c75

Signed-off-by: Javier López Barba <javier@okteto.com>

AgustinRamiroDiaz reviewed Aug 30, 2022

View reviewed changes

cmd/deploy/deploy.go Outdated Show resolved Hide resolved

AgustinRamiroDiaz reviewed Aug 30, 2022

View reviewed changes

cmd/destroy/destroy.go Outdated Show resolved Hide resolved

AgustinRamiroDiaz approved these changes Aug 30, 2022

View reviewed changes

fix: flag description

a495f5a

Signed-off-by: Javier López Barba <javier@okteto.com>

jLopezbarb added 2 commits September 1, 2022 10:11

refactor: rename flag to no-bash

33ebab2

Signed-off-by: Javier López Barba <javier@okteto.com>

fix: deepsource

f3bba3f

Signed-off-by: Javier López Barba <javier@okteto.com>

jLopezbarb requested review from AdrianPedriza and teresaromero September 5, 2022 09:10

teresaromero approved these changes Sep 5, 2022

View reviewed changes

Merge branch 'main' into jlopezbarb/fix-windows-executor

fc46e98

Signed-off-by: Javier López Barba <javier@okteto.com>

jLopezbarb mentioned this pull request Sep 5, 2022

docs: add no bash flag documentation okteto/docs#186

Merged

jLopezbarb merged commit 76e9596 into master Sep 6, 2022

jLopezbarb deleted the jlopezbarb/fix-windows-executor branch September 6, 2022 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use specific terminal #3019

fix: use specific terminal #3019

jLopezbarb commented Aug 19, 2022

codecov bot commented Aug 19, 2022 •

edited

AgustinRamiroDiaz commented Aug 19, 2022

jLopezbarb commented Aug 19, 2022

rberrelleza Aug 20, 2022

jLopezbarb Aug 22, 2022

ifbyol Aug 22, 2022

jLopezbarb Aug 24, 2022

ifbyol Aug 25, 2022

AgustinRamiroDiaz Aug 25, 2022

AgustinRamiroDiaz left a comment

jLopezbarb commented Aug 30, 2022

rberrelleza commented Aug 30, 2022

rberrelleza commented Aug 30, 2022

AgustinRamiroDiaz commented Aug 30, 2022

AgustinRamiroDiaz commented Aug 30, 2022

RinkiyaKeDad commented Aug 31, 2022

ifbyol commented Aug 31, 2022

jLopezbarb commented Aug 31, 2022 •

edited

AgustinRamiroDiaz commented Aug 31, 2022 •

edited

jLopezbarb commented Aug 31, 2022

AgustinRamiroDiaz commented Aug 31, 2022 •

edited

fix: use specific terminal #3019

fix: use specific terminal #3019

Conversation

jLopezbarb commented Aug 19, 2022

Proposed changes

Concerns

codecov bot commented Aug 19, 2022 • edited

Codecov Report

AgustinRamiroDiaz commented Aug 19, 2022

jLopezbarb commented Aug 19, 2022

rberrelleza Aug 20, 2022

Choose a reason for hiding this comment

jLopezbarb Aug 22, 2022

Choose a reason for hiding this comment

ifbyol Aug 22, 2022

Choose a reason for hiding this comment

jLopezbarb Aug 24, 2022

Choose a reason for hiding this comment

ifbyol Aug 25, 2022

Choose a reason for hiding this comment

AgustinRamiroDiaz Aug 25, 2022

Choose a reason for hiding this comment

AgustinRamiroDiaz left a comment

Choose a reason for hiding this comment

jLopezbarb commented Aug 30, 2022

rberrelleza commented Aug 30, 2022

rberrelleza commented Aug 30, 2022

AgustinRamiroDiaz commented Aug 30, 2022

AgustinRamiroDiaz commented Aug 30, 2022

RinkiyaKeDad commented Aug 31, 2022

ifbyol commented Aug 31, 2022

jLopezbarb commented Aug 31, 2022 • edited

AgustinRamiroDiaz commented Aug 31, 2022 • edited

jLopezbarb commented Aug 31, 2022

AgustinRamiroDiaz commented Aug 31, 2022 • edited

codecov bot commented Aug 19, 2022 •

edited

jLopezbarb commented Aug 31, 2022 •

edited

AgustinRamiroDiaz commented Aug 31, 2022 •

edited

AgustinRamiroDiaz commented Aug 31, 2022 •

edited