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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check debug session for multiple container in step. #646

Merged
merged 3 commits into from
Nov 18, 2021

Conversation

sudhirverma
Copy link
Contributor

Fix: #640

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

The problem I can see is that we show all steps together within the terminal and the last one is focussed. So if you don't pay attention to the container you are in, you may execute the debug script in a container which is not the one used currently by tekton.
E.g i have a task with three steps. The first step works fine, the other two should fail because they try to run a not-existing npm command.
When i start debug on this task i see
image

When defining steps in a task, tekton executes them in their order, so first -> second -> third in my case.
There is no reason to show to the user all running containers, we should show user only the step which is ready to be debugged, so the first one which is running after the last completed. In my case, the second step, because the first has been completed and the second is running. This is necessary because, maybe, the previous step is used to set up something for the following. Let's say i'm downloading some files somewhere in the second step and they are used in the third. Why should we allow users to get inside the third step if the second is not completed?

I guess this is the only thing to be improved because if I execute the script in the right order: second step -> third step, it works fine.

Other thought for the future is to switch watching taskruns to pods.

  1. This gives you a protection against problems that tekton is not able to check. The same reason we created the diagnostic action E.g when a pod doesn't start because it can't create a pvc or other errors like this. I think that watching taskrun doesn't help with it. You will see taskrun in running state when the pod will never be created. Not sure though.
  2. Other advantage is that you can re-use the code for pipeline when there will be debug for it as well.

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2021

Codecov Report

Merging #646 (ef32826) into master (7faf1da) will decrease coverage by 0.00%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   68.33%   68.33%   -0.01%     
==========================================
  Files         114      114              
  Lines        6550     6553       +3     
  Branches     1152     1153       +1     
==========================================
+ Hits         4476     4478       +2     
- Misses       2074     2075       +1     
Impacted Files Coverage Δ
src/debugger/debug-tree-view.ts 59.52% <31.57%> (+0.54%) ⬆️
src/cli-command.ts 74.05% <50.00%> (ø)
src/debugger/show-in-terminal.ts 92.85% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7faf1da...ef32826. Read the comment docs.

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM, tested with the pipeline demo, works fine.

@sudhirverma sudhirverma merged commit 685bfbe into redhat-developer:master Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check debug session for multiple container in step.
4 participants