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

Added working workflows that has commenting on forked repo's PR. #481

Merged
merged 8 commits into from
Jan 12, 2021

Conversation

Thomas-Boi
Copy link
Member

This PR is to address this discussion and issue #478.

To develop this, I cloned the repo and opened a PR from this repo into my fork. This is to ensure that the code properly works with PR from forked repo.

Here's the working PR. The working results are near the end. Things take up quite a bit of time because of small Actions error like syntaxes and stuff. However, I'm pretty satisfied with the result.

To test:

  • Fork this repo including this branch
  • Merge this branch into master and develop of the forked repo
  • Open a PR from this repo into the forked branch. Make sure that it has the correct criteria of an icon PR (PR title, svgs in the folder, entry in the devicon.json.
    • You can cherry-pick one of the following commits from the branch testing_files that I put in this repo. It should be 8ce746 for the valid one (you might need to edit the devicon.json file and remove testingicon1 and testingicon2) and 948acf3 for the invalid SVG.
  • Tag it 'bot:peek' and watch the show.

@Thomas-Boi Thomas-Boi linked an issue Jan 11, 2021 that may be closed by this pull request
@Thomas-Boi
Copy link
Member Author

So @pan3793 pointed my attention to a possible event that we can use instead of our current approach.

Personally, I find this approach is cleaner and quicker than our current one. I also wouldn't mind refactoring my code to this new event.

However, here's a caveat that I think you should know:
image

Here's the event link for more details.

Pretty much, if we use this workflow event, we have to make sure the files added is safe before we run the script. I don't think this is too bad but maybe you @amacado might have a different opinion?

Copy link
Member

@amacado amacado left a comment

Choose a reason for hiding this comment

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

I'm fine with this solution, but if you like to rework it feel free ;-) I'm pretty busy right now so I'm unable to test it in depth but from what I can see it should work. Although my heart is bleeding a little for those chained actions. It just not feeling right to have a separated workflow whichs only purpose is to comment the results of another workflow. And this has to be done even twice for each workflow.. Anyway: As long as it works and Github does not provide a more elegant way of solving this I'm glad you found a workaround :)

@Thomas-Boi
Copy link
Member Author

I'm pretty busy right now so I'm unable to test it in depth but from what I can see it should work.

No problem, sorry for mentioning you too often 😅. I'm also starting school again too so I wanted to get this done before I get swarmed.

Since there aren't any PR coming in (probably since people are getting back to work), I think I can take some time and upgrade it to the pull_request_target. However, this is only good if you don't mind the possible security risk.

With bot:peek, we can ensure that the files added are good. However, the check_svgs run on every PR before we can check it. Thus, if someone want to, they can just open a fraudulent PR and get our tokens. We can bypass this by making a special label for it or make it a part of the bot:peek action.

Otherwise, we can just keep it as it is. There's no rush cause we can't really use the workflow until we merge it into master anyway. We can do that for our next build or something.

@amacado
Copy link
Member

amacado commented Jan 12, 2021

@Thomas-Boi in my opinion this solution seems more secure than exposing the tokens (even if it's only the IMGUR secret). It should not be considered best practice to open the project for possible fraud. :) So feel free to merge it. We can also merge to master and get a new release pretty soon.

@Thomas-Boi Thomas-Boi merged commit 7892b03 into develop Jan 12, 2021
@amacado amacado deleted the TB_checkSvg branch January 19, 2021 09:43
@amacado amacado mentioned this pull request Jan 19, 2021
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.

Resource not accessible by integration error
2 participants