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

Create SSH access replacements for calls to docker.exec_run() #362

Conversation

ckunki
Copy link
Contributor

@ckunki ckunki commented Jul 14, 2023

Closes #304

All Submissions:

  • Is the title of the Pull Request correct?
  • Is the title of the corresponding issue correct?
  • Have you updated the changelog?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Are you mentioning the issue which this PullRequest fixes ("Fixes...")
  • Before you merge don't forget to run all tests for all Exasol version, by adding [run all tests] to the commit message
  • Are the CLI usage examples up to date?

@ckunki ckunki requested a review from tkilias July 17, 2023 13:11
ckunki added 2 commits July 17, 2023 17:15
Moved instanciate DockerClient to DockerExecFactory.executor()
Replaced interface DockerClientFactory by implementation based on ContextDockerClientFactory
@ckunki ckunki requested a review from tkilias July 17, 2023 15:46
@ckunki ckunki requested a review from tkilias July 19, 2023 08:56
@ckunki ckunki force-pushed the feature/304-Create_SSH_access_replacements_for_calls_to_docker.exec_run branch from 3731749 to 0748dbb Compare July 19, 2023 15:15
Copy link
Contributor

@tkilias tkilias left a comment

Choose a reason for hiding this comment

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

I didn't see, if we have a integration test for the executor which only run these

ckunki and others added 6 commits July 20, 2023 15:54
…_docker.exec_run' of https://github.com/exasol/integration-test-docker-environment into feature/304-Create_SSH_access_replacements_for_calls_to_docker.exec_run

# Conflicts:
#	exasol_integration_test_docker_environment/lib/base/db_os_executor.py
@ckunki
Copy link
Contributor Author

ckunki commented Jul 21, 2023

I didn't see, if we have a integration test for the executor which only run these

I (again) admire your razor 🔪 eyes. 👀

We have

Up to now we do not have a dedicated integration test for executors and factories ...

... but we have parameterized tests in test/integration/test_bucketfs_upload.py which will cover full integration tests involving executors and factories as well, as soon as the implementation will be updated to actually use them. This is planned in ticket #305.

So if I can convince you to follow my explanation we could go this way.

Otherwise it would not bee too much effort to add dedicated integration tests for executors and factories. The only downside would be some additional runtime required in the CI build.

@tkilias
Copy link
Contributor

tkilias commented Jul 21, 2023

Otherwise it would not bee too much effort to add dedicated integration tests for executors and factories. The only downside would be some additional runtime required in the CI build.

Hm, I am thinking about, if we can make these integration tests cheaper. In the sense, do we really need a database to test this functionality. I think, any ssh capable docker container should work.

@ckunki
Copy link
Contributor Author

ckunki commented Jul 21, 2023

Hm, I am thinking about, if we can make these integration tests cheaper. In the sense, do we really need a database to test this functionality. I think, any ssh capable docker container should work.

OK: As aligned:

client.images.pull("panubo/sshd", tag="1.1.0")
container = client.containers.run(
    name="ssshd", 
    image="linuxserver/openssh-server:9.3_p2-r0-ls123",
    environment=[...])

Appropriate environment variables (see documentation on docker hub):

  • PUBLIC_KEY
  • PUBLIC_KEY_FILE

@ckunki ckunki merged commit 7c0e930 into main Jul 24, 2023
@ckunki ckunki deleted the feature/304-Create_SSH_access_replacements_for_calls_to_docker.exec_run branch July 24, 2023 15:01
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.

Create SSH access replacements for calls to docker.exec_run()
2 participants