Skip to content

[FIX] Make --detach argument explicit to handle future Docker Swarm changes #30

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

Merged
merged 2 commits into from
May 13, 2025

Conversation

PAXANDDOS
Copy link
Contributor

@PAXANDDOS PAXANDDOS commented May 13, 2025

Overview

{9690BED1-4C5D-46DA-951E-8381C9D669E1}

Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.

Docker Swarm will set --detach argument to false in the future, which will break the code unless specified explicitly. I've added another if block to set this argument to true. This will also remove the warning in the logs.

Checklist

  • Verify the Required Checks are Passing
  • Document changes in the README.md (for new features)

@PAXANDDOS PAXANDDOS requested a review from smashedr as a code owner May 13, 2025 12:33
@smashedr
Copy link
Member

The default value of detach is set to true.

  detach:
    description: "Detach Flag"
    required: false
    default: "true"

Therefore if you don't set detach: false the --detach flag will be added to the deploy command.

Is there an edge case where this is not working?

@PAXANDDOS
Copy link
Contributor Author

PAXANDDOS commented May 13, 2025

In the action configuration, yes. But when it comes to handling this flag within src/main.sh, it only adds the flag when the value is false, and does nothing when it's set to true. This results in a warning during execution that the detach argument was not provided and docker will use its own default value which is true but as the warning states, it will change to false in the future.

@PAXANDDOS PAXANDDOS changed the title fix: set explicit --detach [FIX] Make --detach argument explicit to handle future Docker Swarm changes May 13, 2025
@smashedr
Copy link
Member

Looking at this in more detail...

  -d, --detach                 Exit immediately instead of waiting for the stack services to
                               converge (default true)

I think I see the confusion here. Lets break down these 2 scenarios.

Default Behavior

This is the default behavior:

  • detach: true

In this scenario the flag --detach=false is NOT added which means the deploy will detach. ✅

Custom Option

However, if the user does not want to detach and wait for the services to converge, they can set:

  • detach: false

In this scenario the flag --detach=false is added to the deploy command and the deploy will NOT detach. ❌

Conclusion

Based on default behavior and the above options the user has 100% control over the detach option.

That being said, there still can be edge cases, specifically this one that if not already an issue now, will be one day:

  Since --detach=false was not specified, tasks will be created in the background.
  In a future release, --detach=false will become the default.

I was aware of this and knew this flag would need a change eventually. I think your current solution is solid. Since true is the default option, we check if it is true; then if the user sets anything else manually it will be false, which is most likely the desired result of setting this option.

TLDR: Changes look good, ill get this merged and released today. 🚢

@smashedr smashedr merged commit 28e3237 into cssnr:master May 13, 2025
1 check passed
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.

2 participants