-
Notifications
You must be signed in to change notification settings - Fork 7
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: Swift client #472
feat: Swift client #472
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! couple observations
class FliptClientTests: XCTestCase { | ||
|
||
var evaluationClient: FliptClient! | ||
var fliptUrl: String = "https://features-arussellsawsgtest.flipt.cloud" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll want to replace this with env vars
flipt-engine-ffi/src/lib.rs
Outdated
@@ -212,7 +212,7 @@ pub unsafe extern "C" fn initialize_engine( | |||
|
|||
let engine_opts_bytes = CStr::from_ptr(opts).to_bytes(); | |||
let bytes_str_repr = std::str::from_utf8(engine_opts_bytes).unwrap(); | |||
let engine_opts: EngineOpts = serde_json::from_str(bytes_str_repr).unwrap_or_default(); | |||
let engine_opts: EngineOpts = serde_json::from_str(bytes_str_repr).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will cause a panic if invalid which we don't want to do when initializing the engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we panic above though as there are several unwrap calls above this. so ignore me. I've actually been bitten by this before when doing the C# client, where it uses the defaults instead of failing fast so this is probably better they way you have it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes sorry this was from debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good, feel free to keep this change. we need to to do a better job of not panicking in the engine init in the future
@@ -588,6 +591,92 @@ func csharpBuild(ctx context.Context, client *dagger.Client, hostDirectory *dagg | |||
return err | |||
} | |||
|
|||
func swiftBuild(ctx context.Context, client *dagger.Client, hostDirectory *dagger.Directory, opts ...buildOptionsFn) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add the 'integration' tests to test/main.go
as well
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #472 +/- ##
=======================================
Coverage ? 85.48%
=======================================
Files ? 8
Lines ? 3933
Branches ? 0
=======================================
Hits ? 3362
Misses ? 571
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alex Russell-Saw <[email protected]>
d6f582d
to
6b72b5b
Compare
run: | | ||
rustup target add aarch64-apple-ios x86_64-apple-ios aarch64-apple-ios-sim | ||
|
||
- name: Build Engine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this duplicates some work by recompiling the binaries and packaging the xcframework here, but i figured the small time tradeoff is worth the simpler workflow. and this completes before the rest of the integration tests anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor request for client options defaults, otherwise looks GREAT! ty
private var url: String = "" | ||
private var authentication: Authentication? | ||
private var ref: String = "" | ||
private var updateInterval: Int = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this 120? (seconds)
package/ffi/main.go
Outdated
// because of how Go modules work, we need to create a new repo that contains | ||
// only the go client code. This is because the go client code is in a subdirectory | ||
// we also need to copy the ext directory into the tag of this new repo so that it can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could drop or change the messaging here to be swift appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all gravy to me
Signed-off-by: Alex Russell-Saw <[email protected]>
d0ae445
to
57d7aa5
Compare
Signed-off-by: Alex Russell-Saw <[email protected]>
c8909f3
to
0c0a462
Compare
No description provided.