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

Get rid of all_{controllers,runners,ingress_routes} from GetStatus() #1171

Closed
alecthomas opened this issue Apr 4, 2024 · 2 comments
Closed

Get rid of all_{controllers,runners,ingress_routes} from GetStatus() #1171

alecthomas opened this issue Apr 4, 2024 · 2 comments
Assignees
@deniseli
Labels
good first issue Good for newcomers

Comments

@alecthomas
Copy link
Collaborator

These won't scale.
@alecthomas alecthomas added the good first issue Good for newcomers label Apr 4, 2024
@github-actions github-actions bot added the triage Issue needs triaging label Apr 4, 2024
@alecthomas alecthomas mentioned this issue Apr 4, 2024
Open
@alecthomas alecthomas removed the triage Issue needs triaging label Apr 8, 2024
@deniseli deniseli self-assigned this Apr 16, 2024
@deniseli
Copy link
Contributor

waitForControllerOnline in cmd_serve.go still retrieves the controller status via GetStatus(), so I'll leave the controller opt-in where it is to avoid breaking ftl serve. If there's a good workaround to avoid this and pull controller status out of GetStatus(), please let me know :)

@deniseli deniseli mentioned this issue Apr 16, 2024
Merged
deniseli added a commit that referenced this issue Apr 16, 2024
@deniseli
feat: Remove runners and ingress from dal GetStatus (#1280)
b54627d 
Fixes issue #1171

This PR includes some proto field deletions following the guidance here:
https://protobuf.dev/programming-guides/proto3/#deleting

Manually tested `ftl serve` and `ftl status` (the two commands that
reference `StatusRequest`) and got the desired output:
```
$ ftl status
{
  "controllers": [
    {
      "key": "ctr-0-3lky7r59cey54xq",
      "endpoint": "http://localhost:8892"
    }
  ]
}% 
$ ftl serve --recreate
info: Starting FTL with 1 controller(s)
info:controller0: Web console available at: http://localhost:8892
^C% 
$
```
@deniseli deniseli reopened this Apr 16, 2024
@deniseli
Copy link
Contributor

Misunderstood original issue - the desired behavior is to only show the active {controllers,runners,ingress_routes}, not all. Take 2 :)

@deniseli deniseli mentioned this issue Apr 17, 2024
Merged
deniseli added a commit that referenced this issue Apr 17, 2024
@deniseli
GetStatus() should always return just the active controllers/runners/…
cff05fe 
…ingress paths (#1282)

Fixes issue #1171

Generated file changes produced by running `./bin/sqlc generate`

Manually tested by running `ftl status` and `ftl serve`, the two
commands that use `GetStatus()` via `StatusRequest`:

1. In tab 1, ran `ftl dev ./examples/go`. Confirmed everything started
as expected.
2. Opened a new tab 2, ran `ftl status`. Confirmed everything got
returned.
3. Went back to tab 1, Ctrl+C.
4. Went back to tab 2 and ran `ftl status` again. Confirmed got `ftl:
error: unavailable: dial tcp 127.0.0.1:8892: connect: connection
refused`.
5. Went back to tab 1 and ran `ftl serve`. Confirmed serving executed as
expected.
6. Went back to tab 2 and ran `ftl status` again. Confirmed everything
got returned again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deniseli deniseli

Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alecthomas @deniseli