Skip to content

add CI for publishing on the fortran-lang twitter account upon merging a certain PR #147

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

Merged
merged 10 commits into from
Oct 9, 2020
Merged

Conversation

p-costa
Copy link
Collaborator

@p-costa p-costa commented Oct 5, 2020

Hello everyone!

As discussed in the fortran-lang discourse, this PR implements a CI that:

  1. Generates tweets by commenting on a PR
  2. Publishes these tweets when the PR is merged

Please have a look, and feel free to test it by submitting a PR in this repo I created: p-costa/piu-piu-sandbox.

If you are happy with it, to get it to work, one needs to create a twitter app, following the steps similar to those indicated here: github.com/gr2m/twitter-together/blob/master/docs/01-create-twitter-app.md and add the corresponding keys and tokens to this repo.

I should thank the author of find-comment for very promptly considering issues #23 and #24.

The tweets are published using send-tweet-action, as per @LKedward suggestion.

It works as follows:

A comment in the PR starting with the keyword #tweet text will:

  1. remove a previous tweet, if it exists.
  2. add a new tweet to be published by the CI once the PR is merged.

A comment in the PR starting with the keyword #untweet will remove the tweet.

Then, when the PR is merged, the CI will look for a specific keyword ([tweet]) published by the GitHub actions bot, and, if it exists in the PR comment thread, it will be deployed for publication in the fortran-lang twitter account.

@LKedward
Copy link
Member

LKedward commented Oct 5, 2020

Amazing, great work @p-costa! I'll have a look through this later today.

forgot to remove this line I used for debugging
@certik
Copy link
Member

certik commented Oct 5, 2020

Great work. I tested it a bit.

Here are my questions:

https://github.com/p-costa/piu-piu-sandbox/pull/1#issuecomment-703616971

@p-costa
Copy link
Collaborator Author

p-costa commented Oct 5, 2020

Thanks for creating the PR and testing it @certik . I should have mentioned here as well that:

#tweet text will:

  1. remove a previous tweet, if it exists.
  2. add a new tweet to be published by the CI once the PR is merged.

#untweet will remove the tweet.

so that the tweet keyword is not found in other closed pull requests
@milancurcic
Copy link
Member

Thanks, @p-costa! I have 2 questions:

  • Considering that #tweet text will remove a previous tweet if it exists, that means that there can be only one Tweet per PR, correct?
  • What happens if text exceeds 280 characters? Do we get a thread of tweets or is the tweet truncated (or something else)?

@p-costa
Copy link
Collaborator Author

p-costa commented Oct 5, 2020

Hi @milancurcic ,

  • Correct. I assumed that one will not be tweeting too much from here, so 1 tweet per merged PR sounded reasonable.
  • The CI always prints a preview of the tweet showing the character count. So if it is exceeded one will see it and can fix it before merging.

edit: I realize did not fully answer your question: if one merges with a tweet a message with > 280 characters anyway, the send-tweet-action will generate an error. So no threads/truncated tweet.

, because single quotes are more common in sentences; then one can use escape characters to print double quotes in tweets (if something goes wrong it will appear in the preview)"
@p-costa p-costa marked this pull request as draft October 5, 2020 23:23
@p-costa
Copy link
Collaborator Author

p-costa commented Oct 5, 2020

I converted this to draft so that I can polish the implementation tomorrow and make some final tests. Feel free to review it in any case, as it should not change much from my side.

@p-costa p-costa marked this pull request as ready for review October 6, 2020 10:11
@p-costa
Copy link
Collaborator Author

p-costa commented Oct 9, 2020

Just want to comment here that it's ready. But no rush from my side -- I understand that this being a small convenience shouldn't be a high priority thing.

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! I don't see any issues with the workflow which has already been tested and demonstrated. Clearly some work is still required to link to the twitter account.

It looks like anyone can create and modify a tweet, but only maintainers can merge and publish the tweet so I think this workflow works well. Thanks @p-costa 👍

@certik
Copy link
Member

certik commented Oct 9, 2020 via email

@milancurcic
Copy link
Member

It looks like the homework for me is to set up the app and keys:

If you are happy with it, to get it to work, one needs to create a twitter app, following the steps similar to those indicated here: github.com/gr2m/twitter-together/blob/master/docs/01-create-twitter-app.md and add the corresponding keys and tokens to this repo.

I think I'll be able to do it today, if not, then over the weekend.

@p-costa
Copy link
Collaborator Author

p-costa commented Oct 9, 2020

It looks like the homework for me is to set up the app and keys:

Let me know if you run into problems. One thing that was not written in the instructions and I had to do was making sure that the app has read and write permissions:

example

@milancurcic
Copy link
Member

Okay, this is now done. I set the secrets in the Org and allowed them to be shared with this repo. This way we can easily reuse them in other repos when we want.

Should this now be merged?

@milancurcic milancurcic merged commit fcb4666 into fortran-lang:master Oct 9, 2020
@p-costa p-costa deleted the add_twitter_ci branch October 9, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants