Skip to content

Conversation

@ianhi
Copy link
Collaborator

@ianhi ianhi commented Sep 12, 2025

Attempt at separating out tests requiring secrets so that PRs from forks require approval for anything with external srevices secrets.

Currently just manual approval for enabling these. But I think it's also possible with applying a label, however I coudn't get that without essentially duplicating the entire workflow. So for now just keeping it simple. Having a PR label approach can be a future step.

👀
image

@ianhi
Copy link
Collaborator Author

ianhi commented Sep 12, 2025

Ok, two actions for a reviewer:

  1. Are we ok with the number of clicks to approve the workflow? The alternative is to use label-based allowing of these, but that has the downside of once you enable them, someone could push malicious or accidentally dangerous code

If we are happy with this approach then:
2. We need to add these secrets to the ci-with-secrets env, which unfortunately will require copying all of them in manually (you can't just transfer what env they live in). This is probably a task for @jhamman or @paraseba

Also note that there is a limit on the number of possible reviewers for this workflow (6 apparently)

image

@ianhi
Copy link
Collaborator Author

ianhi commented Sep 12, 2025

Seba and I talked about this and decided that this is the best way forward for now, with the option of adding labels to enable as well. WHich I would like to be a follow up PR if I'm going to do it.

The blocker here is the env secrets which I think will require action by @jhamman

For anyone curious to approve you can click th e"requested a deployment" here:

image

@ianhi ianhi marked this pull request as ready for review September 12, 2025 18:32
@dcherian
Copy link
Contributor

Slick, i just clicked through and allowed it

ianhi and others added 11 commits September 26, 2025 14:07
- Split rust-ci tests: basic tests for external PRs, full tests for trusted PRs
- External PRs run unit tests only (--lib) without external service secrets
- Internal PRs and labeled external PRs get full integration testing
- Added label-based approval: maintainers can add 'test-external-services' label
- Added manual dispatch workflow for on-demand external PR testing
- Reusable workflow design prevents code duplication
- Maintains security while enabling external contributions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Split rust-ci into separate jobs: rust-safe (immediate) + rust-external-services (approval required)
- rust-safe: runs unit tests, MinIO/Azurite integration tests, doc tests, examples across all platforms
- rust-external-services: requires 'external-services' environment approval for AWS/R2/Tigris tests
- External PRs get comprehensive feedback immediately while secrets remain protected
- Maintainers can review code changes before approving external service tests
- Uses GitHub's built-in deployment protection with audit trail
- Optimized external tests to only run AWS/R2/Tigris (excludes redundant MinIO tests)
- Maintains backward compatibility with workflow_call for reusability
- Removed separate test-external-pr.yaml (functionality now integrated)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…iciency

- Merge rust-external-services and rust-cron-integration into single job
- For manual approval (PR/push): Run targeted external service tests (AWS, R2, Tigris)
- For cron schedule: Run comprehensive integration tests (all ignored tests)
- Remove duplicate job infrastructure and unnecessary Docker setup
- Maintain same deployment protection for both execution modes

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Update environment name from external-services to ci-with-secrets
- Leverages existing GitHub environment configuration
- Maintains deployment protection for external service tests

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Single rust-external-services job with conditional environment protection:
- External PRs: Require manual approval via ci-with-secrets environment
- Internal PRs & cron: Run automatically without environment protection
- Combines external services and cron integration into unified job
- Maintains test separation: safe tests (MinIO/Azurite) vs external services

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Clean up leftover file from shared workflow approach.
Using single job with conditional environment protection instead.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Remove inputs.ref which only applies to reusable workflows.
Use standard github.event.pull_request.head.sha || github.sha instead.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Remove unused workflow_call section and inputs
- Remove 'labeled' trigger that was for abandoned label-based bypass
- Keep clean regular workflow without reusable workflow leftovers

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ianhi ianhi force-pushed the external-pr-testing-v2 branch from e8a509a to 224be0f Compare September 26, 2025 18:08
@ianhi
Copy link
Collaborator Author

ianhi commented Sep 26, 2025

General stuff

Joe just added secrets to the ci-with-secrets envrinoment. He also moved the S3 test bucket to the earhtmover-sandbox.

We did not set up expiration rules yet, but anyone can do that.

We also moved the bucket and regions to variables instead of secrets.

We need to keep old secrets (not defined in teh environment) around so that they can be used by the cron jobs and internal PRs. We seemingly can't re-use the ci-with-secrets env for those without requiring approval. I think this might be possible with github protection rules, but I wasn't able to figure it out.

This PR

With these changes I just re-ran the tests and it looks like we went from many down to only one failure!

https://github.com/earth-mover/icechunk/actions/runs/18045693619/job/51355816407?pr=1226

Error: ICError { kind: StorageError(S3ListObjectError(ConstructionFailure(ConstructionFailure { source: InterceptorError { kind: ReadBeforeExecution, interceptor_name: Some("ListObjectsV2EndpointParamsInterceptor"), source: Some(BuildError { kind: MissingField { field: "bucket", details: "A required field was not set" } }) } }))), context: SpanTrace [] }


failures:
    test_concurrency_in_aws

I'm not sure why only one of them would fail. @paraseba can you look at that failure and help me figure out if thats a real failure, an issue with the new credentials, or something else?

@ianhi
Copy link
Collaborator Author

ianhi commented Sep 26, 2025

hmm. actually should this be runnign mroe tests? why are there so few now:

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 107 filtered out; finished in 0.00s

     Running unittests src/bin/icechunk/main.rs (target/debug/deps/icechunk-8e9a7a4c877f1b36)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/test_concurrency.rs (target/debug/deps/test_concurrency-10fa0b4024df1d04)

running 1 test
test test_concurrency_in_aws ... FAILED

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