Remove flow containers on exit

[feluelle]

Description

Currently, containers running flow commands are not being removed after completion. This PR removes the containers after completion.

📋 Checklist

• Rebased from the main (or release if patching) branch (before testing)
• Ran make test before taking out of draft
• Ran make lint before taking out of draft
• Added/updated applicable tests
• Tested against Astro-API (if necessary).
• Tested against Houston-API and Astronomer (if necessary).
• Communicated to/tagged owners of respective clients potentially impacted by these changes.
• Updated any related documentation

[codecov]

Codecov Report

Base: 87.12% // Head: 87.12% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (8795ddc) compared to base (a9097d3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #844   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files         103      103           
  Lines        8806     8808    +2     
=======================================
+ Hits         7672     7674    +2     
  Misses        661      661           
  Partials      473      473           

Impacted Files	Coverage Δ
sql/flow.go	93.93% <100.00%> (+0.18%)	⬆️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[feluelle]

@dimberman @pankajkoti @tatiana WDYT? There is no issue for it, but I think it is pretty useful. Otherwise users end up having hundreds of stopped containers. There is only one reason why I think keeping them could be useful and this would be debugging in case of errors, but it will only remove them if the flow command fails - not the container. So we are still able to debug docker errors I think. Not sure if we need the container also to debug flow command errors. Maybe in this case we just suggest users to use the --debug and --verbose flags.

[tatiana]

@feluelle, we can try out the removal and see if it makes our debugging process harder - if it does, we can revert this change.
How do other Astro CLI commands do this?

[pankajkoti]

Perhaps, we can add a debug flag across all commands and if this flag is provided then we don't remove the containers? By default, we can remove all containers. So, if command is failing continuously, we can ask the user to use the --debug flag and then provide us the logs of that failed container, WDYT?

[feluelle]

Interesting idea. We could just add it to the debug/verbose flag we will have on typer cli anyway. Basically so that whenever a user runs with --verbose or --debug (tbd) the command line does not remove the container + the typer cli shows all logs so that we have the option to read all the logs and even test in the affected container.

[feluelle]

I talked with @dimberman about this and he thinks that it would be better to have a separate flag for this - not reusing the debug/verbose flags.

[pankajkoti]

Looks good to me. But, did we decide on whether we will add that separate flag in this PR or later? So as per the current implementation, we're removing all the containers, right?

[feluelle]

Yes, that is correct. @dimberman @tatiana WDYT? Should we have a flag to keep the container alive? I am just wondering if we have a flag which is only for astro-cli it would be a bit hacky since we would have to remove it when passing to sql-cli & we won't be able to see it in any help as we are printing only the sql-cli help 😅 🙈

[tatiana]

If we don't have this flag, and always delete the container, we would still be able to create a container from the docker image and troubleshoot it, right?

Is there anything specific about this container that would make us want to troubleshoot it..? Perhaps the mounting points...?

It would be great if, when passed the verbose flag, we printed the command line users could use to create the container in the same way we are are doing. WDYT?

[feluelle]

Perhaps the mounting points

Yes, this is currently the only thing I could think of we would want to troubleshoot.

It would be great if, when passed the verbose flag, we printed the command line users could use to create the container in the same way we are are doing. WDYT

But we are not using the command line to create the container. We are using the golang sdk.