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

feat: Remove runners and ingress from dal GetStatus #1280

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

deniseli
Copy link
Contributor

@deniseli deniseli commented Apr 16, 2024

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 requested a review from a team as a code owner April 16, 2024 21:15
@deniseli deniseli requested review from matt2e and removed request for a team April 16, 2024 21:15
@github-actions github-actions bot changed the title Remove runners and ingress from dal GetStatus feat: Remove runners and ingress from dal GetStatus Apr 16, 2024
@alecthomas alecthomas mentioned this pull request Apr 16, 2024
@worstell
Copy link
Contributor

lgtm!

@worstell worstell self-requested a review April 16, 2024 21:26
@deniseli deniseli merged commit b54627d into main Apr 16, 2024
11 checks passed
@deniseli deniseli deleted the dli/lighter-getstatus branch April 16, 2024 21:33
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Sorry, I guess the ticket wasn't clear - what it meant was to get rid of the ability to select all controllers/runners, etc. not get rid of the ability to retrieve active runners/controllers altogether. Please revert and adjust.

bool all_controllers = 2;
bool all_ingress_routes = 3;

reserved 1, 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this, as Wes mentioned, we don't bother with preserving proto compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, makes sense

message Route {
string module = 1;
string runner = 2;
string deployment = 3;
string endpoint = 4;
}
repeated Route routes = 5;

reserved 2, 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, makes sense

deniseli added a commit that referenced this pull request Apr 16, 2024
@deniseli deniseli restored the dli/lighter-getstatus branch April 16, 2024 21:54
@matt2e matt2e added the approved Marks an already closed PR as approved label Apr 16, 2024
@deniseli deniseli deleted the dli/lighter-getstatus branch May 14, 2024 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants