new cli publish command #43

sebwoj · 2018-08-23T09:58:05Z

Signed-off-by: Sebastian Wojciechowski [email protected]

In test_good_conf, is it the good idea to check:
mock_publish.assert_called_with(
api.Message(topic=None, body={None:None}), None
)

instead of creating anther patch ->
@mock.patch("fedora_messaging.cli.api.Message")
and as return_value set mock_message, and han check:
mock_publish.assert_called_with(
mock_mnessage, None
)

codecov-io · 2018-08-23T10:19:28Z

Codecov Report

Merging #43 into master will decrease coverage by 2.18%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   95.42%   93.24%   -2.19%     
==========================================
  Files          13       12       -1     
  Lines        1312     1095     -217     
  Branches      158      149       -9     
==========================================
- Hits         1252     1021     -231     
- Misses         45       65      +20     
+ Partials       15        9       -6

Impacted Files	Coverage Δ
fedora_messaging/cli.py	`67.1% <42.85%> (-25.66%)`	⬇️
fedora_messaging/twisted/protocol.py	`92.55% <0%> (-0.66%)`	⬇️
fedora_messaging/api.py	`100% <0%> (ø)`	⬆️
fedora_messaging/config.py	`100% <0%> (ø)`	⬆️
fedora_messaging/testing.py	`100% <0%> (ø)`	⬆️
fedora_messaging/schema_utils.py
fedora_messaging/twisted/consumer.py
fedora_messaging/_session.py	`96.47% <0%> (ø)`
fedora_messaging/twisted/service.py	`100% <0%> (+2.4%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef04f74...e125de0. Read the comment docs.

sebwoj · 2018-08-28T19:43:22Z

How to rerun Travis build?

jeremycline · 2018-08-28T19:53:08Z

A quick way is to amend your latest commit without changing anything - git commit --amend --no-edit - and force pushing to your branch: git push -f

jeremycline · 2018-09-06T22:19:41Z

fedora_messaging/cli.py

+@cli.command()
+@click.option("--publish-exchange", help=_publish_exchange_help)
+@click.option("--body-key", help=_body_key_help)
+@click.option("--body-value", help=_body_value_help)


I wonder if it would be nicer to accept the path to a file that contains JSON. If we did that, we'd need to come up with a format for a JSON-serialized message (message topic, headers, body, etc). That seems generally useful anyway for users who want to create message test fixtures.

What do you think, @abompard?

Yeah I agree, that'd be useful anyway.

sebwoj · 2018-11-21T22:43:39Z

I'm not sure about information provided to user in case of 'loads' failure.
@jeremycline could you take a look on them?
Command 'record' is on the way.

jeremycline

This looks great, thanks for working on this!

I left a number of comments, a few docs suggestions, but nothing major. I had a few questions/idle wonderings as well, don't feel like you've got to address all of them to get this merged, I just wrote down thoughts I had 😄

jeremycline · 2018-11-26T21:35:49Z

docs/fedora-messaging.rst


 ``fedora-messaging consume [OPTIONS]``

    Starts a consumer process with a user-provided callback function to execute
    when a message arrives.

+``fedora-messaging publish [OPTIONS]``
+
+    Loads serialized messges from user-provided file and send all of them to


Suggested change

Loads serialized messges from user-provided file and send all of them to

Loads serialized messages from a file and publishes them to the specified exchange

jeremycline · 2018-11-26T21:38:18Z

docs/fedora-messaging.rst

+publish
+-------
+
+``--publish-exchange``


Since this is a flag on the publish sub-command, I think just having this flag be --exchange is fine.

jeremycline · 2018-11-26T21:54:15Z

fedora_messaging/cli.py

+
+@cli.command()
+@click.option("--publish-exchange", help=_publish_exchange_help)
+@click.option("--file", help=_publish_file_help, required=True, type=click.File("r"))


This might be better as a Click argument rather than an option, although I don't have a great reason for that. Do options let you use - for stdin/stdout?

Another thing to consider is if we want to accept multiple files, or require multiple calls, one per file. I don't have a preference one way or the other and I'm not sure it's going to be a usecase people encounter, so it probably doesn't matter.

jeremycline · 2018-11-26T22:02:59Z

fedora_messaging/cli.py

+        messages = loads(msgs_json_str)
+    except exceptions.ValidationError as e:
+        _log.error("Unable to validate message: %s", str(e))
+        sys.exit(1)


With each of these failure cases exiting as 1, it'll be difficult for scripts to handle specific error scenarios. If a file is invalid, no amount of retrying will help, but if a connection can't be established, waiting a bit might help. I recommend adding different exit codes for the different cases and documenting those codes.

Here I think you can just raise https://click.palletsprojects.com/en/7.x/api/#click.BadArgumentUsage or similar with the message you're logging and it'll do the right thing.

jeremycline · 2018-11-26T22:05:05Z

fedora_messaging/cli.py

+
+
+@cli.command()
+@click.option("--file", help=_record_file_help, required=True, type=click.File("w"))


Again, this might be better as an argument, especially if options don't allow for - to mean stdin/stdout. That way, users can just provide - to get messages printed to the console with this command.

jeremycline · 2018-11-26T22:14:37Z

fedora_messaging/cli.py

+            signal.signal(signal.SIGINT, _signal_handler)
+
+        def __call__(self, message):
+            self.messages.append(message)


If you wanted to be super fancy here, you could set up a click progress bar (https://click.palletsprojects.com/en/7.x/api/#click.progressbar) so users can see how things are progressing. That could also be a follow-up feature, I think.

jeremycline · 2018-11-26T22:15:25Z

fedora_messaging/cli.py

+    if app_name:
+        config.conf["client_properties"]["app"] = app_name
+
+    _log.info("Recording to file %s started.", file)


As mentioned above, you probably want to use click.echo here (or the progress bar)

jeremycline · 2018-11-26T22:22:35Z

fedora_messaging/cli.py

+        sys.exit(1)
+
+    for msg in messages:
+        _log.info("Sending message with topic %s", msg.topic)


Rather than logging I think you want to use https://click.palletsprojects.com/en/7.x/api/#click.echo

jeremycline · 2018-11-26T22:26:41Z

fedora_messaging/cli.py

+        try:
+            api.publish(msg, publish_exchange)
+        except exceptions.PublishReturned as e:
+            _log.error("Unable to publish message: %s", str(e.reason))


Rather than exiting 1 here, I think it'd be good to use a different value than the default "something exploded" error status so scripts can potentially handle error cases differently. It's not a great mapping, but maybe https://docs.python.org/3/library/errno.html#errno.EREMOTEIO?

jeremycline · 2018-11-26T22:28:08Z

fedora_messaging/cli.py

+            sys.exit(1)
+        except exceptions.ConnectionException as e:
+            _log.error("Unable to connect to the queue: %s", str(e.reason))
+            sys.exit(1)


Same deal here, maybe use https://docs.python.org/3/library/errno.html#errno.ECONNREFUSED?

sebwoj · 2018-11-28T23:34:57Z

Thanks for all tips!
Check out my new version of cli.

sebwoj · 2018-11-29T21:42:26Z

Is there any way to get invloved Recorder class in UTs?
Since we are providing hardcoded class to api.consume, is it necessary to catch "ValueError"?

jeremycline

Is there any way to get invloved Recorder class in UTs?

I've not had all my coffee yet this morning so I'm not sure what you mean by this. What does UTs stand for?

Since we are providing hardcoded class to api.consume, is it necessary to catch "ValueError"?

I left a comment inline, but I think we can just call the cli.consume function with the Recorder class so we don't need to duplicate all the error handling code.

jeremycline · 2018-12-05T15:04:25Z

fedora_messaging/tests/unit/test_cli.py

+        )
+        self.assertIn("Sending message with topic test_topic", result.output)
+        mock_publish.assert_called_once()
+        self.assertEqual(mock_publish.call_args_list[0][0][0].topic, "test_topic")


It'd be clearer here to assign mock_publish.call_args_list[0][0][0] to a variable like sent_msg or something. You should also be able to create an expected_msg object with the expected body, topic, etc. and just do self.assertEqual(expected_msg, sent_msg) since the message class implements the equality check.

I think that the equality check between expected_msg and sent_msg will fail because of header["sent-at"] field.

True, although you could set the expected_msg._headers["sent-at"] to be the same as the fixture message

jeremycline · 2018-12-05T15:06:46Z

fedora_messaging/cli.py

+                click.echo("Unable to publish message: {}".format(str(e.reason)))
+                sys.exit(errno.EREMOTEIO)
+            except exceptions.ConnectionException as e:
+                click.echo("Unable to connect to the queue: {}".format(str(e.reason)))


I'd have this be "Unable to connect to the message broker: {}"

jeremycline · 2018-12-05T15:24:29Z

fedora_messaging/cli.py

+def record(exchange, queue_name, routing_key, app_name, limit, file):
+    """Record messages from an AMQP queue to provided file."""
+
+    class Recorder:


So what I'd recommend here is pulling the class out of the function. It's not wrong, but I think it's a little unexpected (I usually only define classes and functions in a parent class/function scope it if is both tiny and relevant only to the internal workings of the parent). Defining this class at the module level should be fine.

After that, I think you can get rid of pretty much all the rest of the code in the record command and have it just call the "consume" cli command here. The only tricky bit is the consume call expects the callback to be a path. I think you could either hard-code the path or add a check to the consume code to see if it's already a class/function. The only downside to hard-coding I see is if the module name changes it'll break, but that will also break APIs so I don't think that's a huge concern.

But the question is how to pass limit and file to Record instance(instance of class outside record func). As I remember consumer api is responsible to make instance of class . Mabye classmethod?

Ah, right. Maybe cli.consume() should accept a class/callable then, since you can create an instance of the class with the limit and file and pass it on.

I think that the problem is in api.comsume func. We pass class type, and api.consume is responsible to make instance.

Also a good point. Given those things maybe leaving it as-is is fine. Or, since testing it here is a bit troublesome, perhaps pull it out into a function called recorder_factory or something which accepts the limit as an argument and returns the class with the limit baked in:

def recorder_factory(limit): class Recorder: def __init__(self): self.limit = limit return Recorder

Perhaps a future enhancement to api.consume would be to allow passing instances of classes. I'd say we shouldn't deal with that right now.

sebwoj · 2018-12-05T21:22:21Z

UT stands for unittests. Since Recorder class was inside recorder func, I have no idea how to test Recorder class(call it in UTs).

pypingou · 2019-05-09T11:13:37Z

@sebwoj Did you see the latest comments?

abompard · 2019-05-09T11:35:00Z

I totally +1 the idea of having one message per line, and if possible to have the loads/dumps top-level functions accept an object and not necessarily a list. I've needed that to setup fedora-messaging in Bodhi CI.

sebwoj · 2019-05-12T09:57:22Z

Please take a look at #172.

jeremycline · 2019-05-13T14:06:42Z

I'm in favor of keeping the work in a single PR. FYI, I've got a lot of work for this done in a local branch, but I was waiting to see what people thought of the design before pushing it.

As for backwards compatibility, I don't have a problem breaking it and releasing 2.0. I don't think anyone was using the dumps/loads APIs yet (it's not documented in the API) so it should be a non-event for everyone involved.

jeremycline · 2019-06-26T19:02:37Z

Okay, this has been rebased on top of the crochet work so it should be ready for review after that's merged.

dustymabe · 2019-06-26T21:09:11Z

docs/fedora-messaging.rst


 ``fedora-messaging consume [OPTIONS]``

    Starts a consumer process with a user-provided callback function to execute
    when a message arrives.

+``fedora-messaging publish [OPTIONS] FILE``
+
+    Loads serialized messages from a file and publishes them to the specified


could we get an example of what a serialized message looks like?

I added a description down in the detailed section below. Please let me know if there's anything that isn't clear in the description.

dustymabe · 2019-06-26T21:52:27Z

Currently if I call publish on a file that has newlines in it (i.e. pretty formatted json) it barfs. If I get rid of the newlines it works.

[vagrant@vanilla-f30 fedora-coreos-koji-tagger]$ cat test
{
    "body": {
        "encouragement": "You're doing great!"
    },
    "headers": {
        "fedora_messaging_schema": "base.message",
        "fedora_messaging_severity": 20,
        "sent-at": "null"
    },
    "id": "null",
    "queue": "new",
    "topic": "nice.message"
}
[vagrant@vanilla-f30 fedora-coreos-koji-tagger]$ cat test | fedora-messaging publish -
Usage: fedora-messaging publish [OPTIONS] FILE

Error: Unable to validate message: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)
[vagrant@vanilla-f30 fedora-coreos-koji-tagger]$ 
[vagrant@vanilla-f30 fedora-coreos-koji-tagger]$ cat test | tr '\n' ' ' | fedora-messaging publish - 
Publishing message with topic nice.message

jeremycline · 2019-06-27T15:21:09Z

I added some docs to the man page about the message format expectations.

dustymabe · 2019-06-28T02:38:16Z

docs/fedora-messaging.rst

+-------
+
+The publish command expects the message or messages provided in ``FILE`` to be
+JSON objects with each message separated by a newline character. The JSON


oh wow.. the newline was required as a delimeter.. did not know that.

dustymabe · 2019-06-28T02:39:03Z

docs/fedora-messaging.rst

+        },
+    },
+    "required": ["topic", "headers", "id", "body", "queue"],
+  }


this is great! I think it would be even better if we had a real example below (i.e. we pass in a real world example message into the fedora-messaging publish -

I went back and look at my comment from yesterday.. we could use that example if we wanted (or tweak it however you'd like)

jeremycline · 2019-06-28T13:57:59Z

On Thu, Jun 27, 2019 at 07:38:17PM -0700, Dusty Mabe wrote: > @@ -91,8 +102,95 @@ configuration file and no options on the command line. ``--exchange`` The name of the exchange to bind the queue to. Can contain ASCII letters, - digits, hyphen, underscore, period, or colon. If one is not specified, the - default is the ``amq.topic`` exchange. + digits, hyphen, underscore, period, or colon. + + Setting this option is equivalent to setting the ``exchange`` setting + in *all* ``bindings`` entries in the configuration file. + + +publish +------- + +The publish command expects the message or messages provided in ``FILE`` to be +JSON objects with each message separated by a newline character. The JSON oh wow.. the newline was required as a delimeter.. did not know that.

Yeah, this isn't great, but the reason was that I wanted you to be able to pipe messages in and they'd be published as they were piped. Requiring everything on one line makes it easy to just loop over incoming lines and parse them as JSON. If you've got a better way to do it I'd love to hear it since I'm not crazy about the current approach either.

dustymabe · 2019-06-28T14:14:22Z

If you've got a better way to do it I'd love to hear it since I'm not crazy about the current approach either.

maybe since the messages are json you can just treat one message as the start and close of curly braces {}. Start reading a message at { and finish at }, repeat.

jeremycline · 2019-06-28T14:54:27Z

On Fri, Jun 28, 2019 at 07:14:23AM -0700, Dusty Mabe wrote: > If you've got a better way to do it I'd love to hear it since I'm not crazy about the current approach either. maybe since the messages are json you can just treat one message as the start and close of curly braces `{}`. Start reading a message at `{` and finish at `}`, repeat.

Perhaps this is safe: ``` message = '' while True: message.append(f.readline()) try: json.loads(message) break except ValueError: continue ``` I can't think of a message where this would exit the loop before getting the complete message, but I haven't thought about it very hard either... I guess it would fail schema validation if that happened anyway. I'll experiment a bit.

jeremycline · 2019-06-28T15:10:59Z

I did think of a serious flaw for that approach. A user puts in some malformed JSON, and now you never exit that loop and you don't know if the user messed up or if it's just going to be right with the next line.

The one message per line approach isn't pretty, but at least it lets you know when you're doing it wrong.

abompard · 2019-08-02T15:54:56Z

Yep, that looks good, anything else you want to add / change @jeremycline ?

These functions now dump and load strings where each line is a JSON object. Previously it created a JSON array of JSON objects, but this approach works better for piping messages into "fedora-messaging publish" from another process that is writing messages as they arrive, one line per message. Signed-off-by: Jeremy Cline <[email protected]>

Add the ability to publish messages using the CLI from a file source or from stdin. Signed-off-by: Sebastian Wojciechowski <[email protected]> [Rebase and rework for the new dump format] Signed-off-by: Jeremy Cline <[email protected]>

Add a new CLI command to record messages from a message broker and write them to a file (or stdout). This was initially intended to be used to collect test fixtures, but can also be chained together with other commands using pipes. Messages are written as they arrive, one message per line, so it's easy to, say, run a command when a message arrives: $ fedora-messaging record - | while read -r line || -n $line; do echo "message received"; done or create a quick (and completely unreliable) bridge between brokers: $ fedora-messaging --conf=broker1.toml record - | \ fedora-messaging --conf=broker2.toml publish Signed-off-by: Sebastian Wojciechowski <[email protected]> [Rebase and refactor] Signed-off-by: Jeremy Cline <[email protected]>

I've updated the PR

jeremycline · 2019-08-02T16:35:55Z

I think I'm happy with it (and we can improve it as we go, obviously). I've dismissed my old review which was blocking the merge, rebased, and I'll go ahead and re-approve this myself so the bot merges it when the tests are done.

jeremycline

✨

sebwoj requested a review from a team as a code owner August 23, 2018 09:58

sebwoj force-pushed the new_cli_publish_command branch from 82ebec3 to 3766d83 Compare August 23, 2018 10:19

sebwoj force-pushed the new_cli_publish_command branch 7 times, most recently from 5efcdca to 0fb9ba8 Compare August 28, 2018 18:31

sebwoj force-pushed the new_cli_publish_command branch 2 times, most recently from aa176bb to c0b1c4b Compare September 1, 2018 10:28

jeremycline reviewed Sep 6, 2018

View reviewed changes

sebwoj force-pushed the new_cli_publish_command branch from c0b1c4b to d341326 Compare November 21, 2018 22:37

sebwoj force-pushed the new_cli_publish_command branch 2 times, most recently from ef4ca61 to 6e585b3 Compare November 24, 2018 11:06

jeremycline reviewed Nov 26, 2018

View reviewed changes

sebwoj force-pushed the new_cli_publish_command branch 2 times, most recently from f04c0a7 to 0c7f507 Compare November 28, 2018 23:32

sebwoj force-pushed the new_cli_publish_command branch from 0c7f507 to 3f67add Compare November 29, 2018 21:31

sebwoj force-pushed the new_cli_publish_command branch 2 times, most recently from 3c324c9 to 892ef22 Compare November 29, 2018 22:05

jeremycline previously requested changes Dec 5, 2018

View reviewed changes

sebwoj force-pushed the new_cli_publish_command branch from 892ef22 to c5ba3fe Compare December 14, 2018 15:09

sebwoj force-pushed the new_cli_publish_command branch from ab6ebcd to 7fe5d1b Compare May 15, 2019 07:47

jeremycline force-pushed the new_cli_publish_command branch from 7fe5d1b to a311b4e Compare June 26, 2019 19:01

dustymabe reviewed Jun 26, 2019

View reviewed changes

dustymabe mentioned this pull request Jun 26, 2019

Create tool for push lockfiles around coreos/fedora-coreos-tracker#205

Closed

jeremycline added the backwards-incompatible Indicates an issue or pull request is backwards-incompatible label Jun 27, 2019

jeremycline force-pushed the new_cli_publish_command branch from a311b4e to 10d7ebe Compare June 27, 2019 21:11

dustymabe reviewed Jun 28, 2019

View reviewed changes

abompard previously approved these changes Aug 2, 2019

View reviewed changes

jeremycline and others added 3 commits August 2, 2019 12:34

cli: Add a publish sub-command

259fb15

Add the ability to publish messages using the CLI from a file source or from stdin. Signed-off-by: Sebastian Wojciechowski <[email protected]> [Rebase and rework for the new dump format] Signed-off-by: Jeremy Cline <[email protected]>

jeremycline dismissed abompard’s stale review via e125de0 August 2, 2019 16:34

jeremycline force-pushed the new_cli_publish_command branch from 17d0940 to e125de0 Compare August 2, 2019 16:34

jeremycline approved these changes Aug 2, 2019

View reviewed changes

mergify bot merged commit a8c9952 into fedora-infra:master Aug 2, 2019

	Loads serialized messges from user-provided file and send all of them to
	Loads serialized messages from a file and publishes them to the specified exchange



		@cli.command()
		@click.option("--file", help=_record_file_help, required=True, type=click.File("w"))

new cli publish command #43

new cli publish command #43

Conversation

sebwoj commented Aug 23, 2018

codecov-io commented Aug 23, 2018 • edited Loading

Codecov Report

sebwoj commented Aug 28, 2018

jeremycline commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebwoj commented Nov 21, 2018

jeremycline left a 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

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

sebwoj commented Nov 28, 2018

sebwoj commented Nov 29, 2018

jeremycline left a 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

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

sebwoj commented Dec 5, 2018

pypingou commented May 9, 2019

abompard commented May 9, 2019

sebwoj commented May 12, 2019

jeremycline commented May 13, 2019

jeremycline commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Jun 26, 2019

jeremycline commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremycline commented Jun 28, 2019 via email

dustymabe commented Jun 28, 2019

jeremycline commented Jun 28, 2019 via email

jeremycline commented Jun 28, 2019

abompard commented Aug 2, 2019

jeremycline commented Aug 2, 2019

jeremycline left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 23, 2018 •

edited

Loading