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

Add option to prevent step from running in forks #153

Closed
corollari opened this issue Mar 12, 2020 · 21 comments · Fixed by #156
Closed

Add option to prevent step from running in forks #153

corollari opened this issue Mar 12, 2020 · 21 comments · Fixed by #156
Assignees
Labels

Comments

@corollari
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When other users fork my repositories this action always fails on their forks because either they don't have the proper secret set up or the cname is already taken (by my upstream repo), this is slightly confusing and it requires me and the users to go to the build page for every build run in order to check if it's this action the one failing or if it's something else.

Describe the solution you'd like
It'd be great if we could toggle an option that would disable this specific action when it's being run in forks.

Describe alternatives you've considered
I can check the origin repository inside an if clause and only run the action if the origin is equal to the upstream name, but while this works it's a bit messy because it requires setting the repository id every time a workflow is set up.

Another alternative would be to just enable allow_failures, but then I wouldn't get alerted when the action fails to deploy the page correctly on the upstream repo.

Overall I don't know if this is something easily doable (the only solution I can think of for detecting if a repo is a fork involves querying github's api) or common enough to warrant it being included as an option.

In any case, I'm willing to implement a solution and create a PR if we manage to come up with an elegant solution to this problem.

@peaceiris
Copy link
Owner

peaceiris commented Mar 12, 2020

There is a quick workaround like the following:

- name: Deploy to GitHub Pages
  if: github.event.repository.fork == false
  uses: peaceiris/actions-gh-pages@v3  # v3.5.2
  with:
    deploy_key: ${{ secrets.ACTIONS_DEPLOY_KEY }}
    # publish_branch: gh-pages  # default
    publish_dir: ./path_to_dir

The deployment on a fork will skip but it is better and friendly for users to detect fork status by this action internally. We should consider whether this action is running on a fork, or not.

When we use the deploy_key or personal_token for deployment, those secret values are not provided to a fork. On the other hand, GITHUB_TOKEN is generated for each user so a fork user can use the github_token option.

- name: Deploy to GitHub Pages
  uses: peaceiris/actions-gh-pages@v3  # v3.5.2
  with:
    github_token: ${{ secrets.GITHUB_TOKEN }}
    publish_dir: ./path_to_dir

This setting works on a fork not only an original repository. Checking whether this action runs on a fork is needed for only deploy_key and personal_token.

Besides, disabling deployment on a fork by default is not good because some users are using this action on their fork for their deployment. (GitHub's fork feature is not for only pull requests.)

Overall I don't know if this is something easily doable (the only solution I can think of for detecting if a repo is a fork involves querying github's api) or common enough to warrant it being included as an option.

Here is a simple example to get a repository metadata about fork status in action side.

import {context} from '@actions/github';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const isForkRepository = (context.payload as any).repository.fork === 'true';

core.info(`isForkRepository: ${isForkRepository}`);

if (isForkRepository) {
  core.info('[INFO] This action runs on a fork');
  // skip deployment
} else {
  // do usual stuff
}

In any case, I'm willing to implement a solution and create a PR if we manage to come up with an elegant solution to this problem.

Thank you. I want more time to think about this specification.

@peaceiris peaceiris added the enhancement New feature or request label Mar 12, 2020
@corollari
Copy link
Contributor Author

Thanks for the detailed response.

What do you think of adding another parameter (for example skip_on_forks) that, when set, will trigger the following behaviour inside the action:

  1. Detect if the cname parameter is set or there is a CNAME file in the publish directory -> If so, skip action
  2. Detect which kind of key is used for the deployment. If anything other than GITHUB_TOKEN is used, skip the action.

Essentially, the action will be skipped if the following boolean expression is true: skip_on_forks && (cname || !github_token).

This would solve the problem you mentioned for users that want to maintain a separate fork (instead of PRs) while providing a way to skip the action for those who just want to send a PR.

Thoughts?

@peaceiris
Copy link
Owner

peaceiris commented Mar 13, 2020

Thank you for suggesting skip_on_forks and it sounds good, but most of the upstream repositories need to add that option for consideration of the forks. It is difficult to say it friendly for upstream users.

I have one idea that does not need to set an option on an upstream repo, has no breaking change to skip deployment on the forks.

How about skipping deployment on forks when the deploy_key or personal_token is set and it is empty?

For users who want to follow their upstream and deploy their own site, this action can print a warning message (e.g. [WARN] action runs on fork and deploy_key is empty).

@corollari
Copy link
Contributor Author

One of the reasons why I proposed adding another option was that it makes it obvious when looking at the yml file that something different happens on forks, but after hearing your latest proposal I like it better.

Is there something else you want to change or think about before moving into implementation?

@peaceiris peaceiris pinned this issue Mar 14, 2020
@peaceiris
Copy link
Owner

peaceiris commented Mar 14, 2020

OK. Please add the new option skip_on_forks. #153 (comment)

See the Makefile, test, and create your commit on a Docker container.

@corollari
Copy link
Contributor Author

Did you change your mind on the skip_on_forks option? I thought we had reached consensus on using your idea over skip_on_forks, have I misunderstood something?

@peaceiris
Copy link
Owner

In the case of adding the skip_on_forks option, we do not have to detect the auth token type. When skip_on_forks is true and this action runs on a fork, this action can skip deployment.

@corollari
Copy link
Contributor Author

I see, why do you prefer that solution over the one you argued for before?

How about skipping deployment on forks when the deploy_key or personal_token is set and it is empty?

@peaceiris
Copy link
Owner

That comes from the name of the skip_on_forks. This option just says "Skip deployment on forks." The behavior should be simple not to confuse users. If skip_on_forks is true, this action should skip all deployment on forks with the three tokens.

My suggestion "skipping deployment on forks when the deploy_key or personal_token is set and it is empty" is a solution without adding a new option.

@peaceiris
Copy link
Owner

My suggestion is based on this previous work nwtgck/actions-netlify#33

@peaceiris
Copy link
Owner

I apologize to you for my lack of clarity in this comment. #153 (comment)

I thought we had reached consensus on using your idea over skip_on_forks, have I misunderstood something?

My suggestion does not need any new options. We can implement our demand without adding a new option like skip_on_forks.

OK. I will implement it and show you my idea.

peaceiris added a commit that referenced this issue Mar 14, 2020
@peaceiris
Copy link
Owner

I opened #156 and released v3.5.4-5. This version can skip deployment on forks when deploy_key or personal_token is empty. There are no new options, the existing users do not need to care about their forks, and this version does not include any breaking changes.

@hamelsmu
Copy link

Wow great work @peaceiris

Amazed by how well maintained this Action is.

@corollari
Copy link
Contributor Author

I apologise, I think I didn't explain myself well before, as I did understand both your solution and the skip_on_forks one, but was just confused over why you had seemed to change your mind over the solution that you liked. To try and shed some light on how my thought process went I'll run through a timeline of it:

  1. I propose skip_on_forks
  2. You propose check_if_auth_empty (identifier I just created for the solution you proposed on Add option to prevent step from running in forks #153 (comment))
  3. I agree that your proposal, check_if_auth_empty, is better. At this point I thought that we would go ahead with that proposal and I'd implement it.
  4. You ask me to implement skip_on_forks. Here's when I got confused over why you chose skip_on_forks instead of check_if_auth_empty.

In any case, everything seems to be solved now and there's already an implementation which I think is perfect (I don't understand why there are several commits adding and removing lib/index.js but ultimately they all cancel each other so it doesn't matter).

@peaceiris
Copy link
Owner

I want to say again, thank you @corollari for suggesting this issue. And I am sorry for confusing you in this thread...

One of the reasons why I proposed adding another option was that it makes it obvious when looking at the yml file that something different happens on forks, but after hearing your latest proposal I like it better.
#153 (comment)

When I read this description, I thought it is good to add the new option skip_on_forks and it looks easy to implement. So I asked you to add skip_on_forks. And I intended to implement my idea and do a comparison between the two prototypes.

In any case, everything seems to be solved now and there's already an implementation which I think is perfect

Thank you. I will merge #156, release it as v3.5.4, and update the v3 tag.

(I don't understand why there are several commits adding and removing lib/index.js but ultimately they all cancel each other so it doesn't matter).

This action and my other actions do not provide the branch execution. I add the lib/index.js for only each release commit. After releasing, I delete it. See release.sh

peaceiris added a commit that referenced this issue Mar 16, 2020
* fix: skip on forks
* chore(release): 3.5.4-6

Close #153
@peaceiris
Copy link
Owner

v3.5.4 has been released and the v3 tag updated.

Thank you.

@peaceiris peaceiris unpinned this issue Mar 16, 2020
@corollari
Copy link
Contributor Author

Thanks! Will start watching the repo and help whenever I can 👍

@fharper
Copy link

fharper commented Jan 27, 2021

Any idea why I get this error with v3.7.3 specially since the repository that I'm using isn't a fork?

If I add if: github.event.repository.fork == false the action isn't running, so there is part of the code that detect my repo as a fork when it's not unless I don't understand something...

@peaceiris
Copy link
Owner

@fharper I cannot understand your problem without your log and settings. You can ask about it at the Discussions · peaceiris/actions-gh-pages first and provide the details.

@fharper
Copy link

fharper commented Jan 28, 2021

@peaceiris my bad! Thanks, I'll share it in the GitHub Discussions.

@github-actions
Copy link
Contributor

This issue has been LOCKED because of it being resolved!

The issue has been fixed and is therefore considered resolved.
If you still encounter this or it has changed, open a new issue instead of responding to solved ones.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants