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

Support PRs from forked repos #1747

Merged
merged 3 commits into from
Jun 15, 2022
Merged

Support PRs from forked repos #1747

merged 3 commits into from
Jun 15, 2022

Conversation

pradnyeshjoshi
Copy link
Collaborator

@pradnyeshjoshi pradnyeshjoshi commented Jun 14, 2022

Description

Triggering execution of unit tests on AzureML requires access to credentials, which are not accessible from a forked repository, causing unit tests triggered by the PRs from outside collaborators to fail with a login error. This PR addresses the issue by using pull_request_target trigger, which allows execution of workflows in the context of a base repository, thus granting access to the AzureML credentials.
Here is a sample PR from a forked repo that can now successfully trigger AzureML tests #1746

Related Issues

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

@pradnyeshjoshi @simonzhaoms great job guys for finding a fix for this

.github/workflows/azureml-unit-tests.yml Show resolved Hide resolved
@miguelgfierro
Copy link
Collaborator

@pradnyeshjoshi another question, do you need this PR to be in main to take effect? let me know and we can do a PR in that case

@pradnyeshjoshi
Copy link
Collaborator Author

@pradnyeshjoshi another question, do you need this PR to be in main to take effect? let me know and we can do a PR in that case

@miguelgfierro once we merge it into staging, people can make PRs into staging from their forks. Since we only make PRs to staging, no immediate need to release to main.

@pradnyeshjoshi pradnyeshjoshi merged commit 469ee29 into staging Jun 15, 2022
@pradnyeshjoshi pradnyeshjoshi deleted the pradjoshi/pr_fork branch June 15, 2022 20:49
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.

2 participants