Skip to content

Add signal query argument for Restart Container command #2186

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
Merged
eddumelendez merged 7 commits into from
Oct 27, 2023
Merged

Add signal query argument for Restart Container command #2186

eddumelendez merged 7 commits into from
Oct 27, 2023

Conversation

@dmmax
Copy link
Contributor

@dmmax dmmax commented Aug 21, 2023 •
edited
Loading

Linked to the issue: #2123

@dmmax dmmax requested a review from a team as a code owner August 21, 2023 05:11
@eddumelendez
Copy link
Member

eddumelendez commented Aug 21, 2023

Can you add a test, please?

@eddumelendez eddumelendez added this to the next milestone Aug 21, 2023
@dmmax
Copy link
Contributor Author

dmmax commented Aug 21, 2023 •
edited
Loading

Hi @eddumelendez

I added an integration test that covers the new functionality.

I also had to add new API versions to the RemoteApiVersion. Let me know if it's ok for you

BTW The test com.github.dockerjava.cmd.ListContainersCmdIT.testStatusFilter is failing for me as well (on the main branch) on the local machine and only increasing the timeout for stopping the container helps. Is it expected behavior?

This was referenced Aug 24, 2023
Merged
Closed
@dmmax
Copy link
Contributor Author

dmmax commented Sep 4, 2023

Hi @eddumelendez
Can you have a look again into the PR? 🙏

eddumelendez
eddumelendez reviewed Oct 26, 2023
View reviewed changes
docker-java/src/test/java/com/github/dockerjava/cmd/RestartContainerCmdImplIT.java Outdated
.withRegistryUrl("https://index.docker.io/v1/")
.build();
try (DockerClient dockerClient = createDockerClient(dockerClientConfig)) {
if (!getVersion(dockerClient).isGreaterOrEqual(RemoteApiVersion.VERSION_1_42)) {
Copy link
Member

@eddumelendez eddumelendez Oct 26, 2023

Choose a reason for hiding this comment

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

I think we can use Assume instead.

@eddumelendez eddumelendez merged commit fd5da16 into docker-java:main Oct 27, 2023
@eddumelendez
Copy link
Member

eddumelendez commented Oct 27, 2023

Thanks for your contribution, @dmmax !

@dmmax dmmax deleted the add_signal_to_restart_container_cmd branch October 29, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eddumelendez eddumelendez eddumelendez approved these changes

Assignees

No one assigned

Labels

type/enhancement

Projects

None yet

Milestone

3.3.4

Development

Successfully merging this pull request may close these issues.

2 participants

@dmmax @eddumelendez