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

[Defect] process-compose project update can't update when processes is empty list #284

Closed
3 tasks done
misuzu opened this issue Dec 10, 2024 · 5 comments
Closed
3 tasks done
Labels
bug Something isn't working done Done, awaiting release

Comments

@misuzu
Copy link

misuzu commented Dec 10, 2024

Defect

Make sure that these boxes are checked before submitting your issue -- thank you!

  • Included the relevant configuration snippet
  • Included the relevant process-compose log (log location: process-compose info)
  • Included a [Minimal, Complete, and Verifiable example] (https://stackoverflow.com/help/mcve)

Version of process-compose: v1.40.1

OS environment: NixOS unstable

Steps or code to reproduce the issue:

  1. create two configs:
cat <<'EOF' > /tmp/test-config-0.yaml
version: "0.5"

processes:
EOF
cat <<'EOF' > /tmp/test-config-1.yaml
version: "0.5"

processes:
  hello:
    command: echo 'Hello World'
  pc:
    command: echo 'From Process Compose'
    depends_on:
      hello:
        condition: process_completed
EOF
  1. run process-compose with the first config
process-compose up --keep-project -f /tmp/test-config-0.yaml
  1. update running process-compose instance using the second config
process-compose project update -f /tmp/test-config-1.yaml

Expected result:

The hello and pc appear in the TUI, the project update exits without errors.

Actual result:

The TUI shows panics for a short time and the project update command exits with an error:

% process-compose project update -f /tmp/test-config-1.yaml
24-12-10 20:55:00.271 FTL Failed to update project error=EOF
% cat /tmp/process-compose-misuzu.log
24-12-10 20:54:55.068 INF Process Compose v1.40.1
24-12-10 20:54:55.068 INF Loaded project from /tmp/test-config-0.yaml
24-12-10 20:54:55.068 INF Global shell command: bash -c
24-12-10 20:54:55.069 INF start http server listening :8080
24-12-10 20:54:55.069 DBG Spinning up 0 processes. Order: []
24-12-10 20:54:55.069 INF Project completed
24-12-10 20:55:00.270 DBG Process hello is new
24-12-10 20:55:00.270 DBG Process pc is new

When starting with /tmp/test-config-1.yaml and updating to /tmp/test-config-0.yaml everything seems to be fine.

@F1bonacc1
Copy link
Owner

F1bonacc1 commented Dec 14, 2024

Hey @misuzu,

Thanks for letting me know about this edge case.
I have a fix, but I am not sure it's what you expect.
Let me explain the issue: When the project is loaded with no processes, the processes value is Nil.
When adding new processes to a Nil value, PC fails.
There are 2 possible fixes:

  1. Initialize the value to be an empty map of processes.
  2. Fail the start of process compose - 0 processes is probably not what the user intended to start and should be warned.

Currently, I am leaning toward option 2 and would've liked to hear your opinion before delivering a fix that might not match your use case.

@F1bonacc1 F1bonacc1 added bug Something isn't working need more info labels Dec 14, 2024
@misuzu
Copy link
Author

misuzu commented Dec 15, 2024

I see, this is actually another yaml gotcha. The processes: means {"processes": null}. For some reason I've expected it to be {"processes": {}}.
With this config everything seems to work as expected:

version: "0.5"

processes: {}

The plan was to start an empty process-compose instance in a systemd unit and then use project update to deploy stuff to it.
I think the first fix would be better and less confusing than how it works now.
With the second one I'd need to add some "default" process just to satisfy the 0 process check, which would be weird...

@F1bonacc1
Copy link
Owner

If your use case is to support an empty process list, I can fail in case of:

version: "0.5"
processes:

And let it run in case of:

version: "0.5"
processes: {}

WDYT?

@misuzu
Copy link
Author

misuzu commented Dec 15, 2024

WDYT?

From the user's point of view, both configurations have empty processes. I would follow the principle of least surprise and make both configurations equivalent

@F1bonacc1 F1bonacc1 added done Done, awaiting release and removed need more info labels Dec 15, 2024
@F1bonacc1
Copy link
Owner

Fixed in v1.46.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working done Done, awaiting release
Projects
None yet
Development

No branches or pull requests

2 participants