Add bazel macros for protobuf_rules_gen by ribrdb · Pull Request #29 · FirebaseExtended/protobuf-rules-gen

ribrdb · 2018-11-09T19:14:28Z

Here's a first stab at adding the BUILD rules.
I'm not sure if you'd want to update the integration test to build using these rules or keep it completely separate.

I didn't include using the firestore emulator here. There's two reasons for that:

The current url for downloading the emulator
(https://firebase.google.com/docs/firestore/downloads/cloud-firestore-emulator.jar)
isn't versioned, so it would be problematic to download it in the WORKSPACE file.
For unrelated reasons I'm stuck using an older version of Bazel and so I can't use the latest rules_nodejs. I'm not sure if it'd be a good idea to give an example using an older version.

Once the emulator jar is available I basically just use a jasmine_node_test and add this to start the emulator:

beforeAll(async () => {
  return new Promise((resolve) => {
    console.log('Starting firestore emulator...');
    const child = child_process.spawn('java', [
      '-jar',
      '../com_google_cloud_firestore_emulator/jar/cloud-firestore-emulator.jar'
    ]);
    child.stdout.on('data', (data) => {
      console.log(`emulator: ${data}`);
      if (/localhost:8080/.test(data)) {
        console.log('Emulator alive!');
        resolve();
      }
    });

    child.stderr.on('data', (data) => {
      console.error(`emulator: ${data}`);
    });
    emulator = child;
  });
});
afterAll(() => {
  emulator.kill();
});

ribrdb · 2018-11-09T19:20:29Z

Hmm, I guess the CI is using an even older version of Bazel than I am. Should I update the rules to work with Bazel 0.9.0, or update the CI to use a newer version?

rockwotj

The emulators are versioned, check here: https://github.com/firebase/firebase-tools/blob/master/src/emulator/constants.js#L27 (although let's do that in another PR)

Can we also move all the *.bzl files into a bazel directory?

README.md

rockwotj · 2018-11-19T15:54:27Z

README.md

+    - firestore_rules_binary combines multiple .rules files (e.g. the auto
+      generated rules with your ACLs that use them)
+    - firestore_rules_library wraps up one or more .rules files so that a 
+      firestore_rules_binary can depend on it.


Looks like this doesn't format nicely. Try s/-/*/?

Ping - these still aren't printing nicely.

example/BUILD

…-rules-gen

ribrdb · 2018-11-27T19:59:46Z

I've been trying to write a test using the firestore emulator, but I'm having lots of problems with it. It seems flaky. The same test case will sometimes pass, sometimes time out, or sometimes return a permission denied error.
I guess for now I'll just add a golden file test.

rockwotj · 2018-11-27T20:19:58Z

Happy to take a look at a version that your experiencing flakiness with.

The emulator should be robust!

- Move bazel definitions to a subdirectory. - Update readme formatting.

ribrdb · 2018-11-27T22:31:52Z

Happy to take a look at a version that your experiencing flakiness with.

The emulator should be robust!

https://github.com/ribrdb/protobuf-rules-gen/blob/emulator/example/rules_test.js

rockwotj

Looking good! A few comments mostly on style. I'll try playing with the emulator tests when I can - hopefully after we merge this PR.

Last nit: can you add newlines to all these files you've added?

rockwotj · 2018-11-28T06:40:13Z

WORKSPACE

 workspace(name = "proto_gen_firebase_rules")

-protobuf_commit = "099d99759101c295244c24d8954ec85b8ac65ce3"
+load(":bazel/repositories.bzl", "protobuf_rules_gen_repositories")


//bazel:repositories.bzl should work and be a more canonical style

rockwotj · 2018-11-28T06:42:40Z

firebase_rules_generator/BUILD


 cc_proto_library(
    name = "firebase_rules_options",
+    visibility = ["//visibility:public"],


Do this libraries need to be public? If anything this should be restricted to this project only.

This is part of my workaround for #25

Can you instead do: package(default_visibility = ["//visibility:public"])

rockwotj · 2018-11-28T06:44:10Z

bazel/repositories.bzl

+        url = "https://github.com/google/protobuf/archive/" + protobuf_commit + ".tar.gz",
+    )
+
+    native.new_http_archive(


Is this needed? Can we depend on we'll know protos from above?

If we can't please leave a comment.

Weird I couldn't get that to work before, but it seems to be fine now.

rockwotj · 2018-11-28T06:45:53Z

bazel/repositories.bzl

+    "google/protobuf/wrappers.proto",
+]
+
+def protobuf_rules_gen_repositories():


Good pattern for deps is to allow someone to override them. See an example here: https://github.com/grpc/grpc/blob/master/bazel/grpc_deps.bzl

rockwotj

Looking good almost there! Thanks for working through this with me.

rockwotj · 2018-11-30T01:11:43Z

BUILD

        "$(locations :testdata)",
    ],
    data = [
+        "//example:testdata/golden.rules",


nit: "//example/testdata:golden.rules",

rockwotj · 2018-11-30T01:12:37Z

BUILD

        "$(location //proto:firebase_rules_options_proto_file)",
        "$(location @com_google_protobuf//:descriptor_proto)",
+        "$(location //example:example.rules)",
+        "$(location //example:testdata/golden.rules)",


"$(location //example/testdata:golden.rules)",

rockwotj · 2018-11-30T03:41:53Z

firebase_rules_generator/BUILD


 cc_proto_library(
    name = "firebase_rules_options",
+    visibility = ["//visibility:public"],


Can you instead do: package(default_visibility = ["//visibility:public"])

rockwotj · 2018-11-30T03:43:49Z

README.md

+    - firestore_rules_binary combines multiple .rules files (e.g. the auto
+      generated rules with your ACLs that use them)
+    - firestore_rules_library wraps up one or more .rules files so that a 
+      firestore_rules_binary can depend on it.


Ping - these still aren't printing nicely.

rockwotj · 2018-11-30T23:50:05Z

Thank you for your contribution!

ribrdb added 3 commits October 31, 2018 09:30

Make build rules public.

5c6d8e3

Add bazel macros for generating firestore security rules.

6a95a4d

Update the readme.

b1bf15e

Use newer bazel version on travis.

a0460ec

rockwotj self-requested a review November 19, 2018 15:43

rockwotj suggested changes Nov 19, 2018

View reviewed changes

Merge branch 'master' of https://github.com/FirebaseExtended/protobuf…

d603f08

…-rules-gen

ribrdb added 2 commits November 27, 2018 13:27

- Add golden test for the bazel example.

e53c3be

- Move bazel definitions to a subdirectory. - Update readme formatting.

Fix merge.

255b502

rockwotj suggested changes Nov 28, 2018

View reviewed changes

Address code review comments.

0ca8611

rockwotj suggested changes Nov 30, 2018

View reviewed changes

Address code review comments.

2064473

rockwotj approved these changes Nov 30, 2018

View reviewed changes

rockwotj merged commit 62ff446 into FirebaseExtended:master Nov 30, 2018

Comments

Conversation

ribrdb commented Nov 9, 2018

Uh oh!

ribrdb commented Nov 9, 2018

Uh oh!

rockwotj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ribrdb commented Nov 27, 2018

Uh oh!

rockwotj commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ribrdb commented Nov 27, 2018

Uh oh!

rockwotj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockwotj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockwotj commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rockwotj left a comment •

edited

Loading

rockwotj commented Nov 27, 2018 •

edited

Loading