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

Bug fix: Allowing tests to run for PRs from forked repos #72

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

venkatajagannath
Copy link
Contributor

@venkatajagannath venkatajagannath commented Sep 24, 2024

The test workflow runs integration tests using an external GKE cluster and therefore requires some credentials.

The integration test currently fails when it is run on PRs from forked repos.

This PR fixes this issue using pull_request_target.

See here for more information:
https://github.com/astronomer/astronomer-cosmos/blob/main/.github/workflows/test.yml
https://datachain.ai/blog/testing-external-contributions-using-github-actions-secrets

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.42%. Comparing base (a783526) to head (a3afe2d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #72   +/-   ##
=======================================
  Coverage   95.42%   95.42%           
=======================================
  Files           5        5           
  Lines         546      546           
=======================================
  Hits          521      521           
  Misses         25       25           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pankajkoti
pankajkoti previously approved these changes Sep 25, 2024
Copy link
Collaborator

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

Approving, but suggestion is to revert the off indentation added for the pull_request: block if it's not needed BEFORE merging the PR. Also note for the future, if we add any job that is expensive, we need to ensure that they have the needs: Authorize dependency set. I verified that we have it currently for the Integration tests job.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@venkatajagannath venkatajagannath merged commit 9f9f683 into main Sep 25, 2024
16 checks passed
@venkatajagannath venkatajagannath deleted the forked_pull_requests branch September 25, 2024 06:25
@tatiana tatiana added this to the Astro Ray Provider 0.3.0 milestone Nov 29, 2024
@tatiana tatiana mentioned this pull request Nov 29, 2024
tatiana added a commit that referenced this pull request Nov 29, 2024
**Breaking changes**

* Removal of ``SubmitRayJob.terminal_states``. The same values are now available at ``ray_provider.constants.TERMINAL_JOB_STATUSES``.
* Simplify the project structure and debugging by @tatiana in #93

In order to improve the development and troubleshooting DAGs created with this provider, we introduced breaking changes
to the folder structure. It was flattened and the import paths to existing decorators, hooks, operators and trigger
changed, as documented in the table below:
  | Type      | Previous import path                        | Current import path                     |
  |-----------|---------------------------------------------|-----------------------------------------|
  | Decorator | ray_provider.decorators.ray.ray             | ray_provider.decorators.ray             |
  | Hook      | ray_provider.hooks.ray.RayHook              | ray_provider.hooks.RayHook              |
  | Operator  | ray_provider.operators.ray.DeleteRayCluster | ray_provider.operators.DeleteRayCluster |
  | Operator  | ray_provider.operators.ray.SetupRayCluster  | ray_provider.operators.SetupRayCluster  |
  | Operator  | ray_provider.operators.ray.SubmitRayJob     | ray_provider.operators.SubmitRayJob     |
  | Trigger   | ray_provider.triggers.ray.RayJobTrigger     | ray_provider.triggers.RayJobTrigger     |


**Features**

* Support using callable ``config`` in ``@ray.task`` by @tatiana in #103
* Support running Ray jobs indefinitely without timing out by @venkatajagannath and @tatiana in #74

**Bug fixes**

* Fix integration test and bug in load balancer wait logic by @pankajastro in #85
* Bugfix: Better exception handling and cluster clean up by @venkatajagannath in #68
* Stop catching generic ``Exception`` in operators by @tatiana in #100
* Stop catching generic ``Exception`` in trigger by @tatiana in #99

**Docs**

* Add docs to deploy project on Astro Cloud by @pankajastro in #90
* Fix dead reference in docs index page by @pankajastro in #87
* Cloud Auth documentation update by @venkatajagannath in #58
* Improve main docs page by @TJaniF in #71

**Others**

Local development

* Fix the local development environment and update documentation by @tatiana in #92
* Enable secret detection precommit check by @pankajastro in #91
* Add astro cli project + kind Raycluster setup instruction by @pankajastro in #83
* Remove pytest durations from tests by @tatiana in #102
* Fix running make docker-run when there is a new version by @tatiana in #99 and #101
* Improve Astro CLI DAGs test so running hatch test-cov locally doesn't fail by @tatiana in #97

CI

* CI improvement by @venkatajagannath in #73
* CI fix related to broken coverage upload artifact by @pankajkoti in #60
* Allow tests to run for PRs from forked repos by @venkatajagannath in #72
* Update CODEOWNERS by @tatiana in #84
* Add Airflow 2.10 (released in August 2024) to tests by @tatiana in #96
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.

Fix integration tests for pull requests from forked repos
4 participants