Skip to content

ocicrypt-rs drop unused env::set_var() - #1653

Open
mythi wants to merge 1 commit into
confidential-containers:mainfrom
mythi:no-envset-ocicrypt
Open

ocicrypt-rs drop unused env::set_var()#1653
mythi wants to merge 1 commit into
confidential-containers:mainfrom
mythi:no-envset-ocicrypt

Conversation

@mythi

@mythi mythi commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@mythi
mythi requested a review from a team as a code owner August 11, 2026 13:25

@Xynnn007 Xynnn007 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I get the point that you want to get rid of set_var for unsafe reasons.

This PR mainly changes the unit tests, and the from_env is a basic routine that ocicrypt(-rs) should support.

It's ok to keep them as-is in unit test codes as they will not be compiled into binary. We can remove set_var parts in real logic. For this PR, you can add from_file test cases to UT which is missed in current codebase.

@mythi

mythi commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

It's ok to keep them as-is in unit test codes as they will not be compiled into binary.

It is true that the PR is triggered by the 2024 edition change. If you read the commit message you notice 003e8ef removes dead code so why not to do the cleanup?

For this PR, you can add from_file test cases to UT which is missed in current codebase.

I believe his is covered by e9f86f1.

@Xynnn007

Copy link
Copy Markdown
Member

Yes. 003e8ef is ok.

about e9f86f1, i am hesitating that from_env is a function so we usually do UT to cover the blackbox, instead of opening it though. As commit message says, it's only one line difference but breaks blackbox. A new separate from_file test would make sense to the code structure.

test_encrypt_decrypt_layer and test_async_decrypt_layer set
OCICRYPT_KEYPROVIDER_CONFIG before running, but test_conf_path was
never actually used: both tests only exercise the jwe key wrapper (via
encrypt_with_jwe / decrypt_with_priv_keys), which is registered
unconditionally and has nothing to do with this env var. The provider.*
wrapper it configures is never invoked, since neither test calls
encrypt_with_key_provider / decrypt_with_key_provider.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>
@mythi
mythi force-pushed the no-envset-ocicrypt branch from e9f86f1 to 509922f Compare August 12, 2026 06:52
@mythi mythi changed the title ocicrypt-rs stop using env::set_var() ocicrypt-rs drop unused env::set_var() Aug 12, 2026
@mythi

mythi commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Your comment on the other commit is fair so I just dropped it.

@Xynnn007

Copy link
Copy Markdown
Member

Thanks for adopting points!

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