Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Build succeeded. ❌ ansible-galaxy-importer FAILURE in 4m 44s (non-voting) |
|
@mandar242 We did duplicate work. I think having one |
I do not think the work is duplicate. Each class should have smaller focus scope (I did copy-paste from the refactoring proposal): SSMSessionManager: Handles everything related to starting and managing the SSM session (e.g., start_session, _connect, exec_command). TerminalManager: Handles terminal configurations (e.g., _prepare_terminal, _wrap_command, etc.). Let me know please if you have any concerns/thoughts. |
Having a more modular approach is generally better for maintainability. As per the proposal, it makes sense to have a separate class instead of consolidating everything into SSMSessionManager. |
There was a problem hiding this comment.
Hey, @mandar242 I pushed some suggestions to restrict the class to the necessary information so that it can be reusable
This comment was marked as outdated.
This comment was marked as outdated.
ad8249b to
be6ec2f
Compare
be6ec2f to
8994d34
Compare
8994d34 to
aa5d89e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
I disagree with @abikouo's suggestion:
Having the connection as a parameter does not make the class reusable; we should pass only the necessary options needed by the class
What you did originally, passing the instance of the connection class, is dependency injection and is usually the better, more flexible way to do things. If you want to continue with your current approach, there are issues around variable initialization you'll need to resolve. Given the timing of when those are initialized in the Connection class, I think it will be difficult to coordinate passing them to the TerminalManager class during initialization.
Makes sense, surely it adds complexity. I’ve reverted the code to use the full connection again for cleaner code. |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 17s (non-voting) |
|
Build succeeded (gate pipeline). ❌ ansible-galaxy-importer FAILURE in 4m 24s (non-voting) |
2c1e8c4
into
ansible-collections:main
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #2277 🤖 @patchback |
…thods (#2270) SUMMARY create TerminalManager class and move related methods ACA-2102 ISSUE TYPE Feature Pull Request COMPONENT NAME connection/aws_ssm ADDITIONAL INFORMATION Reviewed-by: Bikouo Aubin Reviewed-by: Mandar Kulkarni <mandar242@gmail.com> Reviewed-by: Mike Graves <mgraves@redhat.com> Reviewed-by: GomathiselviS <gomathiselvi@gmail.com> (cherry picked from commit 2c1e8c4)
…related methods (ansible-collections#2270) SUMMARY create TerminalManager class and move related methods ACA-2102 ISSUE TYPE Feature Pull Request COMPONENT NAME connection/aws_ssm ADDITIONAL INFORMATION Reviewed-by: Bikouo Aubin Reviewed-by: Mandar Kulkarni <mandar242@gmail.com> Reviewed-by: Mike Graves <mgraves@redhat.com> Reviewed-by: GomathiselviS <gomathiselvi@gmail.com> This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@2c1e8c4
SUMMARY
create TerminalManager class and move related methods
ACA-2102
ISSUE TYPE
COMPONENT NAME
connection/aws_ssm
ADDITIONAL INFORMATION