Added a replay command #334
Conversation
|
I'm not quite sure of the reason behind the failing of these CI tests, but I'm exploring it to see what's causing it🤔 |
|
The tests might not necessarily fail from the changes that you made as they are installability tests but try adjusting the version requirement for the |
|
|
|
This should do it. Also as a rule of thumb - please use the simple present verb in your commit messages. Commit messages describe what a commit does to a repository and not what you did in a commit. |
Got it, 🙌🏾 |
|
One more thing: please allow the user to specify the location of the datagrepper instance (defaulting to |
|
@abompard, I see it makes sense, I'm on it |
|
@abompard I have added the support to user to specify a custom URL |
fedora_messaging/cli.py
Outdated
|
|
||
| @cli.command() | ||
| @click.argument("message_id") | ||
| @click.option("--datagrepper_url", help=_datagrepper_help, default=URL_TEMPLATE) |
There was a problem hiding this comment.
Oh yeah one minor thing: the convention is that option words are separated by dashes, not underscores. So it should be --datagrepper-url. Click will convert that to underscores for the variable name.
fedora_messaging/cli.py
Outdated
| response = requests.get(url, timeout=5) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.HTTPError as e: |
There was a problem hiding this comment.
It's a minor thing (again) but since you're pretty much putting the entire function inside a try/except block, I would just put that block in the calling function instead, something like:
def replay(message_id, datagrepper_url):
try:
message_data = _get_message(message_id, datagrepper_url)
except requests.HTTPError as e:
raise ...There was a problem hiding this comment.
I was wondering what if I remove the try/except from the _get_message function and wrap all these operations in a single try/except block that catches and handles all exceptions? will this be much cleaner?
try:
message_data = _get_message(message_id, datagrepper_url)
api.publish(message.load_message(message_data))
click.echo(f"Message with ID {message_id} has been successfully replayed.")
except requests.HTTPError:
raise click.ClickException(f"Could not find message with ID {message_id}")
except Exception as e:
raise click.ClickException(f"Failed to replay message: {e}")There was a problem hiding this comment.
Very good question.
I'd prefer keeping the try/except blocks specific to what we want to handle, for example we know how to handle HTTPErrors for the _get_message() call, so we handle it, but we may not want to catch the generic exception there. So, multiple blocks seems cleaner to me.
I'm familiar with unit tests, I will check the existing ones and follow the same workflow. |
|
Test cases results:
|
fedora_messaging/cli.py
Outdated
| response = requests.get(url, timeout=5) | ||
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.HTTPError as e: |
There was a problem hiding this comment.
Very good question.
I'd prefer keeping the try/except blocks specific to what we want to handle, for example we know how to handle HTTPErrors for the _get_message() call, so we handle it, but we may not want to catch the generic exception there. So, multiple blocks seems cleaner to me.
|
OK great! Only the |
Actually, I thought I fixed it already 😅 |
|
Ah, yeah, the unit test needs to be adjusted as well. |
|
OK thanks! Could you rebase and solve the conflicts? If you have any issues doing that please ask. |
I was wondering also If I should squash all these commits in one? |
Yeah that would be preferable. |
Signed-off-by: freedisch <freeproduc@gmail.com>
|
Merged, thanks again for your contribution. |
Description of changes
Message.load_messageto create a Message object from the JSON data, ensuring all headers and properties are preserved, and then publishes it usingapi.publish.