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

Issue with Optimize-Bot on forked repo PR #614

Closed
Thomas-Boi opened this issue May 18, 2021 · 2 comments · Fixed by #624
Closed

Issue with Optimize-Bot on forked repo PR #614

Thomas-Boi opened this issue May 18, 2021 · 2 comments · Fixed by #624
Labels
devops Use this label for devops related enhancements

Comments

@Thomas-Boi
Copy link
Member

Thomas-Boi commented May 18, 2021

Hello guys,

I discovered an issue with the optimize-bot as seen in #610. The checkout and commit action acts differently when the PR is from a forked repo.

The commit action calls for the checkout action to checkout the github.head_ref when the trigger is pull_request. Unfortunately, this cause the action to checkout the PR branch's name but look for it in the devicons/devicon repo => does not exist. The problem is described here.

Removing the github.head_ref would cause the commit action to fail entirely but everything else works.

I'd like to ask you guys what you think of the following potential solutions:

  • Changing the commit action into a create-pr action. This is similar to what we already have in the build-fonts action.

    • The target will be the PR branch:
      • Pro: Don't need to close the branch and keep everything in the same PR
      • Con: Might need permission of the owner to merge. If the owner has abandoned the PR, this can be problematic. However, if the owner has abandoned the PR, we'll have to take it over nonetheless and can PR into our own branch.
    • The target will be develop:
      • Pro: We can merge it in whenever.
      • Con: We'll have to close the old PR manually. PR will also be cluttered
  • Keep the commit action but rather than using files-changed action, manually search for files added based on the PR title.

    • Pro: Easy to do. peek-bot is using this.
    • Con: Hacky. Depends on PR title to be correct. Will need a wrapper Python script to run the npm script but that's not too bad.
  • Run the optimize script another way: on merge, when building, etc...

    • Pro: Batch things together and save time
    • Con: need more work.

I'm thinking of doing option 2. We haven't have an issue with peek-bot yet. While it's a bit hacky, it works well and I won't have to do much to add this functionality.

What do you guys think?

@Thomas-Boi Thomas-Boi added the devops Use this label for devops related enhancements label May 18, 2021
@Thomas-Boi
Copy link
Member Author

Thomas-Boi commented May 19, 2021

Ok, after doing some work, I realized that option 2 does not work. I misunderstood/did not comprehend the docs for the commit action. Turns out that we will need to make the contributor enables the Action on their forked repo. See this for more details.

I'm thinking that optimizing the SVGs all at once when we do a release (option 3) might be better idea. Either that or I look into the PR action (option 1). I would like to optimize the SVGs right away so people can start using the SVGs in develop ASAP. However, I don't like the cons of option 1 so I'm leaning towards option 3.

@Panquesito7 @amacado let me know what you guys prefer.

@Thomas-Boi
Copy link
Member Author

I'll be going ahead on option 3. Even the commit action recommends it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Use this label for devops related enhancements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant