Update custom filter example and add CI #389

markmandel · 2021-09-02T04:10:20Z

Update the custom filter example to the new API surface, and also setup CI so that it ensures that it always compiles against dev Quilkin, and therefore will stay in sync going forward.

Next I want to block out pieces of code and inject them into our docs via mdbook when updating https://googleforgames.github.io/quilkin/main/book/filters/writing_custom_filters.html as well, rather than have them copy pasted.

Work on #373

markmandel · 2021-09-02T04:10:55Z

build/Makefile

@@ -47,7 +47,10 @@ version:
 	@echo $(package_version)

 # Run all tests
-test: ensure-build-image
+test: ensure-build-image test-quilkin test-examples


Split up quilkin tests from examples tests here into separate targets, in case someone only wants to test one at a time.

markmandel · 2021-09-02T04:11:39Z

examples/quilkin-filter-example/Cargo.toml

-prost-build = "0.7"
+prost-build = "0.8"
+
+[workspace]


Had to add this [workspace] field here, otherwise I would get the error:

Error during execution of cargo metadata: error: current package believes it's in a workspace when it's not: -- I'm not sure I really get cargo workspaces tbh, so if you have any feedback here, I would appreciate it.

All cargo projects (even ones with one crate) are workspaces, starting from the Cargo.toml and recursively throughout the directory. So if you have a project with multiple crates, cargo needs to figure out if it's part of the parent workspace, or if it's in a new workspace (this will determine what crates to build, where the target directory is, etc). One way to do that is to add [workspace] in the Cargo.toml, which is explicitly saying "This is the start of a new workspace". The other option is using the members and exclude fields in the parent Cargo.toml's [workspace]. I would probably add one of those fields to the parent instead, depending on how you want to build.

members = ["examples/quilkin-filter-example", ...], add it to the members field, which means it will be built as part of cargo build --workspace.

exclude = ["examples/quilkin-filter-example", ...], add it to the exclude field, which means it will not be built as part of cargo build --workspace, and you need to build it explicitly yourself.

Thanks for the detailed explanation - that helped a lot!

I'm definitely leaning towards having it be seperate, and that way if you are developing against quilkin you don't have to process the example of every build and test.

I just tried using exclude in quilkin/Cargo.toml, and I'm still getting the error:

Error during execution of `cargo metadata`: error: current package believes it's in a workspace when it's not: current: /home/markmandel/workspace/quilkin/examples/quilkin-filter-example/Cargo.toml workspace: /home/markmandel/workspace/quilkin/Cargo.toml this may be fixable by adding `examples/quilkin-filter-example` to the `workspace.members` array of the manifest located at: /home/markmandel/workspace/quilkin/Cargo.toml Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.

Seems like leaving [workspace] in the example Cargo.toml is the only way.

markmandel · 2021-09-02T04:12:00Z

examples/quilkin-filter-example/Cargo.toml

-prost-types = "0.7"
+# If lifting this example, you will want to be explicit abou the Quilkin version, e.g.
+# quilkin = "0.2.0"
+quilkin = { path = "../../" }


This was the best way I could think of to ensure that the code stayed in sync. Is there a better way?

I think this is reasonable. This code is never being published to crates.io, so using paths is totally fine.

markmandel · 2021-09-02T04:13:00Z

examples/quilkin-filter-example/src/main.rs

+        })
+    }
+}
+


Should/could we using include_proto! here instead? I tried it, and it wanted me to have tonic as a dependency, so I'm assuming that's why we avoided it in the example?

I've no issue with using tonic, if you depend on quilkin, you already depend on tonic indirectly.

Agreed! Got it to work! Update coming soon.

quilkin-bot · 2021-09-02T04:25:49Z

Build Succeeded 🥳

Build Id: 8db477dd-898b-4ca6-8c65-e11d3f8bea13

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/389/head:pr_389 && git checkout pr_389
cargo build

quilkin-bot · 2021-09-02T04:32:31Z

Build Succeeded 🥳

Build Id: e2d7317c-106c-44ad-acfc-78a3b29ac51c

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/389/head:pr_389 && git checkout pr_389
cargo build

XAMPPRocky · 2021-09-02T06:50:45Z

examples/quilkin-filter-example/Cargo.toml

-prost-types = "0.7"
+# If lifting this example, you will want to be explicit abou the Quilkin version, e.g.
+# quilkin = "0.2.0"
+quilkin = { path = "../../" }


I think this is reasonable. This code is never being published to crates.io, so using paths is totally fine.

XAMPPRocky · 2021-09-02T06:58:46Z

examples/quilkin-filter-example/Cargo.toml

-prost-build = "0.7"
+prost-build = "0.8"
+
+[workspace]


All cargo projects (even ones with one crate) are workspaces, starting from the Cargo.toml and recursively throughout the directory. So if you have a project with multiple crates, cargo needs to figure out if it's part of the parent workspace, or if it's in a new workspace (this will determine what crates to build, where the target directory is, etc). One way to do that is to add [workspace] in the Cargo.toml, which is explicitly saying "This is the start of a new workspace". The other option is using the members and exclude fields in the parent Cargo.toml's [workspace]. I would probably add one of those fields to the parent instead, depending on how you want to build.

members = ["examples/quilkin-filter-example", ...], add it to the members field, which means it will be built as part of cargo build --workspace.

exclude = ["examples/quilkin-filter-example", ...], add it to the exclude field, which means it will not be built as part of cargo build --workspace, and you need to build it explicitly yourself.

XAMPPRocky · 2021-09-02T07:00:09Z

examples/quilkin-filter-example/src/main.rs

+        })
+    }
+}
+


I've no issue with using tonic, if you depend on quilkin, you already depend on tonic indirectly.

Update the custom filter example to the new API surface, and also setup CI so that it ensures that it always compiles against dev Quilkin, and therefore will stay in sync going forward. Next I want to block out pieces of code and inject them into our docs via mdbook when updating https://googleforgames.github.io/quilkin/main/book/filters/writing_custom_filters.html as well, rather than have them copy pasted. Work on googleforgames#373

quilkin-bot · 2021-09-02T17:55:33Z

Build Succeeded 🥳

Build Id: 32bac2b1-18fd-4908-8407-c4482d0a6c5a

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/389/head:pr_389 && git checkout pr_389
cargo build

iffyio · 2021-09-06T15:59:31Z

examples/quilkin-filter-example/src/main.rs

+        let greeting = self
+            .require_config(args.config)?
+            .deserialize::<Config, ProtoGreet>(self.name())?;
+        Ok(Box::new(Greet(greeting.greeting)))


this is now out of sync with the doc, we'll need to update that to match

That'll be my next job! I'll take a stab at pulling the code samples from this code directly as well via mdbook. 👍🏻

quilkin-bot · 2021-09-07T21:38:49Z

Build Succeeded 🥳

Build Id: 1c3df0c3-d462-44cf-a0c9-7546530da149

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/389/head:pr_389 && git checkout pr_389
cargo build

markmandel added area/tests Unit tests, integration tests, anything to make sure things don't break kind/cleanup Refactoring code, fixing up documentation, etc area/build-tools Development tooling. labels Sep 2, 2021

markmandel requested review from XAMPPRocky and iffyio September 2, 2021 04:10

google-cla bot added the cla: yes label Sep 2, 2021

github-actions bot added the size/s label Sep 2, 2021

markmandel commented Sep 2, 2021

View reviewed changes

markmandel force-pushed the examples/filter-updates branch from cb6afde to 57c27f9 Compare September 2, 2021 04:14

XAMPPRocky approved these changes Sep 2, 2021

View reviewed changes

markmandel added 2 commits September 2, 2021 10:23

Review updates.

c2b92b0

markmandel force-pushed the examples/filter-updates branch from 57c27f9 to c2b92b0 Compare September 2, 2021 17:30

markmandel mentioned this pull request Sep 2, 2021

XDS Management Server #360

Merged

3 tasks

iffyio reviewed Sep 6, 2021

View reviewed changes

Merge branch 'main' into examples/filter-updates

7d25fe4

markmandel merged commit e412049 into googleforgames:main Sep 8, 2021

markmandel deleted the examples/filter-updates branch September 8, 2021 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update custom filter example and add CI #389

Update custom filter example and add CI #389

markmandel commented Sep 2, 2021

markmandel Sep 2, 2021

markmandel Sep 2, 2021

XAMPPRocky Sep 2, 2021 •

edited

Loading

markmandel Sep 2, 2021

markmandel Sep 2, 2021

XAMPPRocky Sep 2, 2021

markmandel Sep 2, 2021

XAMPPRocky Sep 2, 2021

markmandel Sep 2, 2021

quilkin-bot commented Sep 2, 2021

quilkin-bot commented Sep 2, 2021

XAMPPRocky Sep 2, 2021

XAMPPRocky Sep 2, 2021 •

edited

Loading

XAMPPRocky Sep 2, 2021

quilkin-bot commented Sep 2, 2021

iffyio Sep 6, 2021

markmandel Sep 7, 2021

quilkin-bot commented Sep 7, 2021

Update custom filter example and add CI #389

Update custom filter example and add CI #389

Conversation

markmandel commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XAMPPRocky Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quilkin-bot commented Sep 2, 2021

quilkin-bot commented Sep 2, 2021

Choose a reason for hiding this comment

XAMPPRocky Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quilkin-bot commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quilkin-bot commented Sep 7, 2021

XAMPPRocky Sep 2, 2021 •

edited

Loading

XAMPPRocky Sep 2, 2021 •

edited

Loading