Spin Factors - #2519
Closed
lann wants to merge 22 commits into
Closed
Conversation
Merged
lann
force-pushed
the
factors
branch
3 times, most recently
from
May 23, 2024 13:03
86f4bb7 to
f1dc5ed
Compare
rylev
reviewed
Jun 4, 2024
| .component_allowed_hosts | ||
| .get(ctx.app_component().id()) | ||
| .cloned() | ||
| .context("missing component allowed hosts")?; |
Collaborator
There was a problem hiding this comment.
This error message is a bit weak. Perhaps:
Suggested change
| .context("missing component allowed hosts")?; | |
| .with_context(|| format!("missing allowed outbound hosts configuration for component {}", ctx.app_component().id()))?; |
rylev
reviewed
Jun 5, 2024
rylev
reviewed
Jun 7, 2024
rylev
reviewed
Jun 10, 2024
| } | ||
| } | ||
|
|
||
| pub trait SelfInstanceBuilder: 'static {} |
Collaborator
There was a problem hiding this comment.
This and DefaultInstanceBuilder are nice, but I'm not convinced they're worth it. It seems like they might make things harder to understand in order to save a few lines of boilerplate...
lann
commented
Jun 10, 2024
| /// The default implementation returns an empty schema, which accepts any | ||
| /// configuration. | ||
| fn runtime_config_json_schema(&self) -> impl serde::Serialize { | ||
| HashMap::<(), ()>::new() |
Collaborator
Author
There was a problem hiding this comment.
This is somewhat of a placeholder for now, thinking about how to support #2529 (comment)
lann
commented
Jun 11, 2024
lann
commented
Jun 12, 2024
Comment on lines
+99
to
+101
| // TODO: This seems fine, but then again I don't understand why `FieldOffset`'s | ||
| // own `Sync`ness depends on `U`... | ||
| unsafe impl<T: RuntimeFactors, F1: Factor, F2: Factor> Sync for StateGetter2<T, F1, F2> {} |
Collaborator
Author
There was a problem hiding this comment.
A little spooky, but less spooky than what I had before...
Collaborator
There was a problem hiding this comment.
Does this need to stay here? If so, can you try rewriting the comment as to why this is actually safe to do?
Collaborator
Author
There was a problem hiding this comment.
I have a fork of field-offset that fixes it, but I'm not sure that anyone is actually maintaining upstream 😬.
If that PR doesn't get merged I'm not sure I even want to keep the dependency...
rylev
reviewed
Jun 13, 2024
lann
force-pushed
the
factors
branch
2 times, most recently
from
June 20, 2024 21:03
eeeb140 to
6fbd5b7
Compare
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
And shorten some generic params. Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Also address a bunch of feedback. Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Also sneak in Factor::runtime_config_json_schema Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Collaborator
Author
|
Closing this in favor of working on the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref #2518
This has been turned into the
factorsbranch