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

feat: trigger crate #418

Merged
merged 22 commits into from
May 3, 2022
Merged

feat: trigger crate #418

merged 22 commits into from
May 3, 2022

Conversation

Mossaka
Copy link
Contributor

@Mossaka Mossaka commented Apr 30, 2022

Built on top of the draft PR #415 and discussed in issue #402

Notable difference: added a type TriggerExtra that encapsulate Router for HttpTrigger or subscriptions for RedisTrigger

@lann @radu-matei

@Mossaka
added a trigger crate and a Trigger trait
a5f50f3 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
Merge branch 'main' into jiazho/trigger
4de6528
@Mossaka
implement trigger crate
e040189 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
implement trigger crate (refactor)
3b4504f 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
reduce usued imports
eca7da4 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix clippy issues
0b0d7fc 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix clippy issues
c078e8a 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix format issues
9bd177b 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix format issues
58d8eeb 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix format issues
066e8c8 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
fix format issues
083a521 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
removed Redis trigger new()
5d24176 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
cargo fmt
c686a3e 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
cargo clippy
0e69a40 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
cargo fmt
e4bc68c 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka Mossaka mentioned this pull request May 1, 2022
Open
Mossaka and others added 4 commits May 1, 2022 10:44
@Mossaka
cargo fmt & cargo clippy
2eaa0c4 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@lann
Remove Trigger::TriggerExtra associated type
474b0e9 
- Add ComponentMap::get_by_id to support simplified Router impl
- Refactor Router to build from ComponentMap & return component IDs
@lann
Rename spin_trigger::get_default_trigger -> build_trigger_from_app
af992a1 
Better match Rust naming conventions.

Also, use this function in spin_testing.
@lann
Remove ComponentMap impl
4b54f9c 
Since it no longer "looks like" it has CoreComponent keys, replace it
with a type alias to a HashMap.
@lann
Copy link
Collaborator

lann commented May 2, 2022

@Mossaka: Reviewing this PR inspired me to make a bunch of changes I've been thinking of doing anyway, one benefit of which being that I was able to simplify some of the new trigger crate code. Take a look: https://github.com/fermyon/spin/pull/418/files/2eaa0c48c09a2f16a4211a169dd047bc2fefc7c1..4b54f9cc1883f3b1e2bff0b2c438a1ad26f1b667

@lann lann mentioned this pull request May 2, 2022
Closed
@Mossaka
Copy link
Contributor Author

Mossaka commented May 2, 2022

@lann I really like your approach that removes the dependency on Application

@Mossaka
Copy link
Contributor Author

Mossaka commented May 2, 2022

I will have an update to resolve the conflicts

@Mossaka
resolved conflicts
1892697 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
itowlson
itowlson approved these changes May 3, 2022
View reviewed changes
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

LGTM - delighted to see this

crates/manifest/src/lib.rs Show resolved Hide resolved
src/commands/up.rs Outdated Show resolved Hide resolved
fibonacci1729
fibonacci1729 approved these changes May 3, 2022
View reviewed changes
crates/http/src/lib.rs Outdated
.ok_or_else(|| anyhow!("Expected HTTP configuration for component {}", id))
})?;
#[derive(Clone)]
pub struct HttpRuntimeConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Anything but just "Runtime"......maybe HttpTriggerRuntimeConfig? ......pretty please

Copy link
Contributor

@fibonacci1729 fibonacci1729 May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OR HttpTriggerConfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatya think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are just too many configs...

  1. HttpTriggerConfiguration
  2. HttpConfig
  3. and this HttpRuntimeConfig

How about HttpRuntimeConfig -> HttpTriggerExecutionConfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be one for a subsequent blitz on naming - I don't even know the difference between the first two let along the new one - we should do a pass for figure out their different responsibilities and give them clearly distinct names that clearly express those differences, but this PR might not be the place for that, not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. My understanding is that

  1. The first one is for top level trigger struct "trigger = { type = "http", base = "/" }
  2. The second one is for component trigger struct
[component.trigger]
route = "/hello"
  1. The third one is for CLI aguments: http has --listen address, --log-dir and --tls-cert/key

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that we shouldn't try to name everything perfectly in this PR. The names were already pretty confusing.

@Mossaka
renamed runtime -> execution
3822f58 
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
@Mossaka
Copy link
Contributor Author

Mossaka commented May 3, 2022

I had a commit 3822f58 that changes some struct's names

mostly Runtime -> Execution

@lann
Copy link
Collaborator

lann commented May 3, 2022

I am going to resolve conflicts and merge shortly.

@lann lann merged commit 986170d into fermyon:main May 3, 2022
@lann lann mentioned this pull request May 3, 2022
Closed
This was referenced May 3, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radu-matei radu-matei radu-matei left review comments

@fibonacci1729 fibonacci1729 fibonacci1729 approved these changes

@itowlson itowlson itowlson approved these changes

@lann lann lann approved these changes

@michelleN michelleN Awaiting requested review from michelleN

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Mossaka @lann @itowlson @fibonacci1729 @radu-matei