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

feat: add feature flag to enable/disable support for Anomcred (backend job and API) #1491 #1492

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

FabioPinheiro
Copy link
Contributor

Add a configuration feature flag to enable or disable support for certain type of credentials like Anomcred

@FabioPinheiro FabioPinheiro requested a review from a team as a code owner January 9, 2025 12:06
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Unit Test Results

104 files  ±0  104 suites  ±0   21m 32s ⏱️ + 4m 16s
882 tests ±0  873 ✅  - 1  8 💤 ±0  1 ❌ +1 
889 runs  ±0  880 ✅  - 1  8 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit a9ef730. ± Comparison against base commit 37a8f41.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Jan 9, 2025

Coverage Status

coverage: 48.691% (+0.05%) from 48.642%
when pulling ad10dcb on feat/feat_flag_for_anomcred
into 37a8f41 on main.

@github-actions github-actions bot added the pollux label Jan 9, 2025
@FabioPinheiro FabioPinheiro changed the title feat: add feature flag to enable/disable support for Anomcred (backend job) #1491 feat: add feature flag to enable/disable support for Anomcred (backend job and API) #1491 Jan 10, 2025
@yshyn-iohk
Copy link
Member

Good job, @FabioPinheiro

What do you think about using the feature flags in the ZIO layer bootstrapping logic instead of in the business logic.
I afraid, that it might be harder to test the logic and additional feature flags might make the code hard to maintain.

Signed-off-by: FabioPinheiro <[email protected]>
@FabioPinheiro
Copy link
Contributor Author

Good job, @FabioPinheiro

What do you think about using the feature flags in the ZIO layer bootstrapping logic instead of in the business logic. I afraid, that it might be harder to test the logic and additional feature flags might make the code hard to maintain.

I did exactly that discussion with @mineme0110 yesterday. We are slightly in favor of being in the ZIO environment. But to keep consistent, I decided to pass the flags as arguments (as it was already done in most places)

@FabioPinheiro
Copy link
Contributor Author

So now the integration tests are failing as expected. How can we fix this?
The easier way is to add the configuration to behave us before. What about the new expected behavior but he default?

Present Proof Protocol #1.Holder presents anoncreds credential proof to verifier FAILED
    java.lang.AssertionError at LastResponseInteraction.kt:33

@mineme0110
Copy link
Contributor

So now the integration tests are failing as expected. How can we fix this? The easier way is to add the configuration to behave us before. What about the new expected behavior but he default?

Present Proof Protocol #1.Holder presents anoncreds credential proof to verifier FAILED
    java.lang.AssertionError at LastResponseInteraction.kt:33

can we not run integration test with flag enabled

@yshyn-iohk
Copy link
Member

I agree with @mineme0110.
Let's keep the e2e tests with all flags (it should be possible to set the corresponding env variables)
At the same time, we need to add negative use cases that prove disabling logic works well.

@FabioPinheiro
Copy link
Contributor Author

So that is the "easier way". Although I have no idea how to configure the integration test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants