-
Notifications
You must be signed in to change notification settings - Fork 2
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
Mastodon support #6
Conversation
also add a cosmetic closing paren in one message
still untested but it seems like it should work...
Signed-off-by: Chad Dougherty <crd@acm.org>
Mastodon
Signed-off-by: Chad Dougherty <crd@acm.org>
add an example job
Signed-off-by: Chad Dougherty <crd@acm.org>
Mastodon
Signed-off-by: Chad Dougherty <crd@acm.org>
Signed-off-by: Chad Dougherty <crd@acm.org>
Mastodon
Signed-off-by: Chad Dougherty <crd@acm.org>
no mastodon branch
Signed-off-by: Chad Dougherty <crd@acm.org>
a couple more missed sections
Signed-off-by: Chad Dougherty <crd@acm.org>
also cleanup whitespace Signed-off-by: Chad Dougherty <crd@acm.org>
Mastodon
twitter->mastodon in readme better help text for --deploy-mastodon Signed-off-by: Chad Dougherty <crd@acm.org>
Will take a look soon! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition! I added some comments and suggestions, and I'll suggest here what I typically do for this kind of action - if you can use this branch in your intended repository (to test) and then link the working action and toots, that will be the final sanity check that all is well.
Thanks for contributing this!
action.yml
Outdated
mastodon_access_token: | ||
description: API key generated for the user account to tweet | ||
required: false | ||
nastodon_api_base_url: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this perhaps have a default to mastodon.social's space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there really is any good default. It's not centralized like Twitter. I admit that I'm not Mastodon expert but I have seen hundreds of Mastodon instances cropping up in the course of the great exodus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair! I think we are possibly setting the user up to make a mistake - e.g., assume a default and want to deploy but then they don't have all the metadata, and it just posts to twitter/slack. How about we add a check (before any of that happens) to protect them - the entire thing should fail (before doing any posts) if they intended to deploy to mastodon but forgot an envar. E.g., something like:
if args.deploy_mastodon and not mastodon_client:
sys.exit("Mastodon deploy requested, but environment variables are missing.")
elif args.deploy_mastodon and mastodon_client:
<what you currently have>
We would probably want this pattern for the others (if we don't have it yet) but that's out of scope for this PR if you don't want to add it. In the meantime, let's do these checks earlier so we don't have partial posting. After that + the test run I think we should be good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check that you are suggesting already happens in get_mastodon_client()
at lines 127-129, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh, missed that! Yes you are spot on. So the remaining item is to use this branch in a repo somewhere to demonstrate that it works (and we can preview the toots to make sure they look okay - e.g., I remember for Twitter/Slack we had to do some work to get the images / links to expand).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All set! And when your work goes in and the branch over at US-RSE is updated I can delete that old one. It should have been merged like a year ago, sorry for the oversight!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think this is ready to merge. Please check out the last action on my fork:
https://github.com/crd477/jobs-updater/actions/runs/3797373182
and the resulting toots:
https://mastodon.sdf.org/@crd
Let me know if you think it looks OK. Once it goes into main here, I'll see about fixing the usrse actions separately.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that run is updated with commits here? I still see the warning about set-output, checkout@v2, and then the incorrect input param. Is that the right link?
Also - tests need to pass here - so if it's trying to use mastodon (but cannot) we shouldn't be testing it here (because we cannot!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test Slack and Twitter from here? Can a similar thing be done for Mastodon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have a slack webhook defined (I'm not there anymore but you could check the US-RSE slack) and note that the workflow is just explicitly to check slack (no twitter). For Twitter we had to do the same we are doing here - test somewhere else with an actual token before merging here.
typo: addiction->addition Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
typo: nastodon->mastodon Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
Signed-off-by: Chad Dougherty <crd@acm.org>
Signed-off-by: Chad Dougherty <crd@acm.org>
Signed-off-by: Chad Dougherty <crd@acm.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo I just caught, and I'm checking where we use set-output (will need to update that).
ARGH! typo: mastdodon->mastodon sorry... Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
@crd477 is there a reason you set test to false here? That's why the action is failing (we don't have those credentials in the test repo)
|
No, there was no real reason. |
set test to true Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think I missed a set-output in the python file - I'll fix that after this!
@crd477 thanks for your patience here! The PR here looks good - we do need one final run on your branch (with the updates here) to check that outputs / warnings have gone away (and that the toot is still successful). No worries if you want to pick up another day - we had a lot of back and forth today! |
Sorry - I meant the workflow where you post to mastodon? I think this is just the tests (that are here, just slack) running on your branch? I can merge as is and we can then test after, but it's usually better to see that everything works before that. |
My Mastodon account has already been censored by the server administrators for using a bot. I was insulted enough that I simply deleted the account so I can't test it any more anyway. |
oh my gosh! I'm so sorry. Let's just merge, and the update to US-RSE (I assume with some credential) can be the test, and totally OK if we need to come back and do another round! I'll hold off on drafting a release until we do that, but you should be able to use main branch. Ping me on the PR so I know when it's merged and can delete the old branch! And seriously, thank you for your hard work today! I'm so sorry about your account, I feel partially responsible for that. |
Is there someone I can ping on your server to apologize, and see if we can get your account back? |
Nah, don't worry about it. I've never been on any other social media platform before and I honestly haven't seen any redeeming qualities in Mastodon that make me want to start now. I'm 95% sure I never would've used that account for anything other than testing this patch. |
Anyway, I'm sorry you aren't happy - hopefully we can make it up in the future with a less arduous / more fun PR review! |
@@ -27,6 +27,21 @@ def write_file(content, filename): | |||
with open(filename, "w") as fd: | |||
fd.write(content) | |||
|
|||
def set_env_and_output(name, value): | |||
""" | |||
helper function to echo a key/value pair to the environement file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed this while looking for something else: environement -> environment.
Sorry I didn't notice it while we were iterating on the PR.
add support for posting updates to Mastodon using the
Mastodon.py
moduleFunctionally similar to the Twitter support