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

Only run merge-scheduler workflow if it's not a fork #3267

Merged
merged 6 commits into from
Feb 11, 2022

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Feb 11, 2022

@julieg18 julieg18 marked this pull request as draft February 11, 2022 00:00
@shcheklein shcheklein temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:00 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:12 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:35 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:37 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:48 Inactive
@julieg18 julieg18 temporarily deployed to dvc-org-update-merge-sc-x93p4i February 11, 2022 00:48 Inactive
@julieg18 julieg18 changed the title Only run Merge Scheduler if it's not a fork Only run merge-scheduler workflow if it's not a fork Feb 11, 2022
@julieg18 julieg18 self-assigned this Feb 11, 2022
@julieg18 julieg18 marked this pull request as ready for review February 11, 2022 00:52
@@ -11,13 +11,15 @@ on:
jobs:
merge_schedule:
runs-on: ubuntu-latest
if:
${{ github.event.pull_request.head.repo.full_name == github.repository }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checks if the repo name of the pull request equals the github repo. I tested it with a fork (#3270) and it appears to work correctly!

@julieg18 julieg18 requested a review from a team February 11, 2022 00:56
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the quick fix!

@julieg18 julieg18 merged commit 450d7d9 into master Feb 11, 2022
@julieg18 julieg18 deleted the update-merge-schedule-workflow branch February 11, 2022 12:39
@shcheklein
Copy link
Member

Is it expected? Is it bc we were experimenting with the GH workflow? Ot does it create a new check / status every hour?

Also, how does the merge work - who is the author of the merge?

Also, do we know why fork didn't work?

Screen Shot 2022-02-11 at 10 19 08 PM

@julieg18
Copy link
Contributor Author

julieg18 commented Feb 14, 2022

Is it expected? Is it bc we were experimenting with the GH workflow? Ot does it create a new check / status every hour?

@shcheklein Yes, the multiple checks are expected. The check runs every time a pr is opened, edited, or receives new commits.

Also, how does the merge work - who is the author of the merge?

The action uses the github rest api to merge the pr. So I believe the action itself is the author! It should look something like this:

image

Also, do we know why fork didn't work?

Yes. The github action doesn't run on forks for security reasons.

@shcheklein
Copy link
Member

@julieg18 thanks!

@shcheklein Yes, the multiple checks are expected. The check runs every time a pr is opened, edited, or receives new commits.

🤔 hmm, why don't we see the same for the link checker, unit tests, etc? (it's the first time I see the checks being duplicated this way).

anyways, we can't keep it this way I think - it'll be exploding on some long-running PRs

The action uses the github rest api to merge the pr. So I believe the action itself is the author! It should look something like this:

does it mean that anyone can come and create a PR and it'll be automatically merged (if they put a time to merge it in the description)?

Yes. The github action doesn't run on forks for security reasons.

what error message do we see in this case? can it be changed - per repo or per specific action? and the most important - do we want this to be changed (somewhat related to the previous question I think)

@julieg18
Copy link
Contributor Author

🤔 hmm, why don't we see the same for the link checker, unit tests, etc? (it's the first time I see the checks being duplicated this way).

The same check can be duplicated when it is configured to run for different events. For example, our link-check-deploy runs for two different events and appears twice on a commit:

Screen Shot 2022-02-14 at 4 23 37 PM

But most of our checks only need to run for one event and thus, aren't getting duplicated like merge-scheduler

anyways, we can't keep it this way I think - it'll be exploding on some long-running PRs

Agreed! Maybe we can look for github action alternatives or update the configuration for this one.

does it mean that anyone can come and create a PR and it'll be automatically merged (if they put a time to merge it in the description)?

I believe that it can't be merged unless it has atleast one approving review. But I'll create a test right now to be sure.

what error message do we see in this case? can it be changed - per repo or per specific action? and the most important - do we want this to be changed (somewhat related to the previous question I think)

Here's the error message:

image

I don't think it can be changed. What do you mean exactly by "do we want this to be changed"? Changed to what exactly?

@shcheklein
Copy link
Member

Sorry for the confusion. can it be changed == can we change the behavior, the way it behaves on forks.

@julieg18
Copy link
Contributor Author

Sorry for the confusion. can it be changed == can we change the behavior, the way it behaves on forks.

Oh, ok! Thanks for the clarification! No, I don't think so. The merge-schedule action doesn't offer a way to change this behavior.

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.

3 participants