Skip to content

Conversation

@alexrashed
Copy link
Member

Motivation

Unfortunately, it turns out that the fix to avoid the execution of test pipelines which rely on secrets implemented in #9383 is not correctly working, as we saw with the workflows executed on the following PRs:

It turns out that github.repository evaluates to the PR target repo (which in this case is localstack/localstack.
However, we want to make sure that the source and the target of the PR are not the same repo.
This PR tries to fix this condition by checking the pull request head target as described in https://github.com/orgs/community/discussions/26829.

Changes

  • Prevent the execution of the "Community Tests against Pro" (which inherently rely on the usage of secrets which cannot and shouldn't be provided for runs on untrusted code) by fixing the condition.
  • Fixing the S3 image tests by only using the secrets if they are available.

Testing

Unfortunately, this cannot really be tested properly as long as it's in a PR. Happy for any feedback / tips on how to test this though!

@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Mar 1, 2024
@alexrashed alexrashed requested a review from bentsku March 1, 2024 07:40
@alexrashed alexrashed self-assigned this Mar 1, 2024
@github-actions
Copy link

github-actions bot commented Mar 1, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 20s ⏱️
390 tests 339 ✅  51 💤 0 ❌
780 runs  678 ✅ 102 💤 0 ❌

Results for commit 5b34be1.

@coveralls
Copy link

Coverage Status

coverage: 83.799% (-0.006%) from 83.805%
when pulling 5b34be1 on fix-contribution-execution
into dc061cf on master.

@github-actions
Copy link

github-actions bot commented Mar 1, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 26m 47s ⏱️ +32s
2 667 tests ±0  2 418 ✅ ±0  249 💤 ±0  0 ❌ ±0 
2 669 runs  ±0  2 418 ✅ ±0  251 💤 ±0  0 ❌ ±0 

Results for commit 5b34be1. ± Comparison against base commit dc061cf.

Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, seems sensible. Probably not worth it to test it extensively before merging. We can just open a fork PR after the fact and iterate over it if needed.

@alexrashed alexrashed merged commit f0993ac into master Mar 1, 2024
@alexrashed alexrashed deleted the fix-contribution-execution branch March 1, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants