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

Previews in PR Conversation #224

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

IstoraMandiri
Copy link
Contributor

@IstoraMandiri IstoraMandiri commented Oct 5, 2022

UPDATED

This PR implements a number of fixes and features to do with preview tweets, which makes collaboration in twitter-together repos a lot more streamlined.

Primarily, this PR fixes #223, adding support for the action to create (and update) a generated comment that renders a preview the latest committed tweet(s). Here's an example, using this PR's build. ethereumclassic/tweets-etc_network#92.

I have refactored a fair amount of the pull-request code path to separate concerns, splitting the create-check-run logic and making the report type more explicit.

It fixes #219 by generating previews for all Front Matter types.

It also fixes #218 by updating the documentation for the workflow file to check out the head of the pull request so that the action can check the presence of the media files to be uploaded.

Test cases for PRs have also been added for all recently introduced Front Matter types and threading.

For repos currently using this action, the old behavior will remain, but for those using pull_request_target, the action will create comments instead of check runs to report the result of new tweets. It can be enabled for pull_request PRs from local branches using a documented environment variable.

Important: I have updated the README documentation with this optional config change, a fix for #218 by checking out the PR branch, as well as explaining the pull_request_target security implications. I would like someone with more knowledge of GitHub actions to review this to ensure it's correct guidance. I'm confused as to what happens if an outside contributor submits an innocent PR, which runs, and then updates their branch to modify the workflow file. Could this leave the repository vulnerable?

Once the README has been reviewed, I can update the relevant config docs elsewhere.

This PR does not include any version changes or new distributions, and should be backwards compatible, but perhaps a v4 major bump might be considered guarantee these significant changes don't break anything.

If this is approved and merged, I'll submit another PR for adding Tweeted URLs to the merged PR comment thread.

@IstoraMandiri IstoraMandiri changed the title fix: PR previews for new frontmatter types fix: PR previews for new Front Matter types Oct 5, 2022
@IstoraMandiri
Copy link
Contributor Author

I have just added another commit to this PR that fixes #221 by adding an additional check to parse-tweet-file-content.js for retweets and replies that will throw if they are invalid.

I have also split out the URL parsing function so we can extend if to support other formats such as tweet IDs or malformed URLs.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

https://github.com/IstoraMandiri/twitter-together-testing/runs/8713977079

do you think we could link to the media for preview, or even show a preview in the markdown?

Otherwise fantastic work 👍🏼

// TODO allow differently formatted URLs and tweet ids ?
// https://github.com/twitter-together/action/issues/221

// TODO: Should we check if the referenced tweet actually exists?
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a great idea, can you file a follow up issue?

@IstoraMandiri
Copy link
Contributor Author

Thanks for those suggestions @gr2m. Yes, I'll update this PR shortly to generate better previews of media. I had left it generic for now as I wasn't sure whether it supported videos, but for now I can at least render images.

@gr2m
Copy link
Contributor

gr2m commented Oct 8, 2022

let me know when it's ready for another review

@IstoraMandiri
Copy link
Contributor Author

I've just updated this branch to render images as so:

https://github.com/IstoraMandiri/twitter-together-testing/runs/8860835815

@IstoraMandiri
Copy link
Contributor Author

Ah, hold on, need to deal with nested filenames.

@IstoraMandiri IstoraMandiri marked this pull request as draft October 13, 2022 04:16
@IstoraMandiri
Copy link
Contributor Author

Okay, nested media files in sub directories are now supported.

(see dog)
https://github.com/IstoraMandiri/twitter-together-testing/runs/8867019970

@IstoraMandiri IstoraMandiri marked this pull request as ready for review October 13, 2022 10:47
@IstoraMandiri IstoraMandiri marked this pull request as draft October 14, 2022 10:27
@IstoraMandiri IstoraMandiri changed the title fix: PR previews for new Front Matter types Comment Previews Oct 14, 2022
@IstoraMandiri IstoraMandiri changed the title Comment Previews Previews in PR Conversation Oct 14, 2022
@IstoraMandiri
Copy link
Contributor Author

IstoraMandiri commented Oct 15, 2022

@gr2m @MattIPv4 I have updated the top comment of this thread, I think it is ready for a review, but not quite ready to merge pending advice on the documentation.

@IstoraMandiri IstoraMandiri marked this pull request as ready for review October 17, 2022 12:26
@gr2m
Copy link
Contributor

gr2m commented Oct 17, 2022

I'll be very busy this week, I try my best to review but no promises. Feel free to go ahead without me if you both agree

@IstoraMandiri
Copy link
Contributor Author

IstoraMandiri commented Oct 30, 2022

FYI, the merge_schedule check on this PR seems to be failing for this and other PRs, seems to be misconfigured.

@gr2m
Copy link
Contributor

gr2m commented Nov 23, 2022

I wont be able to review the pull request in depth. I can look at specific things and happy to merge it and fix follow up problems as they occur.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants