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.

Sign up for GitHub

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

Jump to bottom

Added tests #257

Merged
merged 16 commits into from
Mar 26, 2024
Merged

Added tests #257

merged 16 commits into from
Mar 26, 2024

Conversation

ckunki
Copy link
Contributor

@ckunki ckunki commented Mar 19, 2024

closes #252

tkilias
tkilias requested changes Mar 19, 2024
View reviewed changes
test/integration/test_create_dss_docker_image.py Outdated Show resolved Hide resolved
test/integration/test_create_dss_docker_image.py Outdated Show resolved Hide resolved
@ckunki
Fixed review findings
b52f76d 
[run-notebook-tests]
@ckunki ckunki temporarily deployed to AWS_SAGEMAKER March 21, 2024 14:20 — with GitHub Actions Inactive
tkilias
tkilias reviewed Mar 21, 2024
View reviewed changes
test/docker/container.py Outdated Show resolved Hide resolved
tkilias
tkilias reviewed Mar 21, 2024
View reviewed changes
test/docker/container.py Outdated Show resolved Hide resolved
tkilias
tkilias requested changes Mar 22, 2024
View reviewed changes
Copy link
Collaborator

@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.

see my comments

@ckunki
Reverted function container
5072c85 
container_context now uses container()
@ckunki ckunki requested a review from tkilias March 22, 2024 15:34
@ckunki ckunki temporarily deployed to AWS_SAGEMAKER March 25, 2024 07:18 — with GitHub Actions Inactive
tkilias
tkilias reviewed Mar 25, 2024
View reviewed changes
test/integration/test_create_dss_docker_image.py Outdated

@contextmanager
def added_group(request, base_image, gid, group_name):
# dss_docker_image.image_name
Copy link
Collaborator

@tkilias tkilias Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can be a fixture, because get_unassigned_gid can be fixture

tkilias
tkilias reviewed Mar 25, 2024
View reviewed changes
test/integration/test_create_dss_docker_image.py Outdated
)


def altered_inspector(request, image_name: str, socket_on_host: Path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be also a fixture, because added_group can be fixture

Copy link
Collaborator

Choose a reason for hiding this comment

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

needs better name

@ckunki
Fixed review findings
6a5fc95 
Refactored SocketInspector etc.
Moved some functions and classes to separate file
@ckunki ckunki merged commit 70850eb into main Mar 26, 2024
6 checks passed
@ckunki ckunki deleted the refactoring/#252-Add_tests_for_access_to_Docker_socket branch March 26, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkilias tkilias tkilias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add tests for access to Docker socket
2 participants
@ckunki @tkilias