Skip to content
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

AMS sending silently ignores empty messages #261

Open
tofu-rocketry opened this issue Sep 18, 2023 · 1 comment · May be fixed by #267
Open

AMS sending silently ignores empty messages #261

tofu-rocketry opened this issue Sep 18, 2023 · 1 comment · May be fixed by #267
Labels
bug good first issue Good for newcomers
Milestone

Comments

@tofu-rocketry
Copy link
Member

Following #247, I noticed that we're silently ignoring empty messages.

ssm/ssm/ssm2.py

Line 493 in 0ec0b31

argo_id = self._send_msg_ams(text, msgid)

_send_msg_ams will return None for an empty message and the code here will log as if the message was sent:

ssm/ssm/ssm2.py

Line 495 in 0ec0b31

log_string = "Sent %s, Argo ID: %s" % (msgid, argo_id)

We should wrap that in an if...else and log a different message if the message wasn't sent, perhaps a warning.

@tofu-rocketry tofu-rocketry added bug good first issue Good for newcomers labels Sep 18, 2023
@RedProkofiev
Copy link
Contributor

_send_msg_ams in its current implementation will not return a None under effectively any circumstance. A newline from a touch'd, empty file or some random 0's or more importantly the empty string will all render as "something", because only None can be None. We could try to handle this by handling for the empty string, making it return a None and not bother sending if the file itself has no characters bar newlines, but I'm reluctant to filter any more. After that, the if...else handling around 493 should work fine. Do you think that's worth implementing?

@tofu-rocketry tofu-rocketry added this to the 3.5.0 milestone Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

2 participants