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: add proto for language plugin service #3060

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Oct 9, 2024

No description provided.

@matt2e matt2e requested review from a team and alecthomas as code owners October 9, 2024 23:04
@matt2e matt2e requested review from a team and wesbillman and removed request for a team October 9, 2024 23:04
@ftl-robot ftl-robot mentioned this pull request Oct 9, 2024
@matt2e matt2e mentioned this pull request Oct 9, 2024
38 tasks
@matt2e matt2e requested a review from stuartwdouglas October 9, 2024 23:10
@matt2e
Copy link
Collaborator Author

matt2e commented Oct 9, 2024

@stuartwdouglas a review would be helpful having had the experience of adding jvm support. Would this protocol work well for building an external jvm plugin? (not yet aimed at live reloads)

@matt2e matt2e added the skip-proto-breaking PRs with this label will skip the breaking proto check label Oct 9, 2024

// LanguageConfig contains any metadata specific to a specific language.
// These are stored in the ftl.toml file under the same key as the language (eg: "go", "java")
google.protobuf.Struct language_config = 7;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this strongly typed using google.protobuf.Any? We'd likely need some mechanism to pass the descriptor to FTL from the language plugin so it knows what to expect, so I'm not sure it's worth the hassle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeh I tried with google.protobuf.Any but we needed the proto definition for each language config, which was why I backtracked to just a generic struct. I didn't think it would be worth the effort for now.

// FTL may ignore this event if it does not match FTL's current build context and state.
// If the plugin decides to cancel the build because another build started, no failure or cancellation event needs
// to be sent.
message AutoRebuildStarted {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking that it might be good to have a sequence diagram in the design doc for these flows, WDYT?

// Each time this call is made, the Build call must send back a corresponding BuildSuccess or BuildFailure
// event with the updated build context id with "is_automatic_rebuild" as false.
rpc BuildContextUpdated(BuildContextUpdatedRequest) returns (BuildContextUpdatedResponse);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very cool!

@matt2e matt2e force-pushed the matt2e/language-proto branch from 6e855b6 to 4bd40de Compare October 10, 2024 00:51
@matt2e matt2e force-pushed the matt2e/language-proto branch from 4bd40de to eb8cae7 Compare October 10, 2024 00:52
@matt2e matt2e enabled auto-merge (squash) October 10, 2024 00:52
@matt2e matt2e merged commit f85767a into main Oct 10, 2024
95 checks passed
@matt2e matt2e deleted the matt2e/language-proto branch October 10, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-proto-breaking PRs with this label will skip the breaking proto check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants