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

Env var support in agent configs #89

Draft
wants to merge 3 commits into
base: feat/VDX-308
Choose a base branch
from

Conversation

sanderPostma
Copy link
Contributor

No description provided.

@sanderPostma sanderPostma marked this pull request as ready for review November 15, 2023 10:00
@sanderPostma sanderPostma marked this pull request as draft November 15, 2023 10:01
Copy link
Contributor

@nklomp nklomp left a comment

Choose a reason for hiding this comment

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

Please change the delimiter to ||, or better yet to ?? So that is it more natural to read.

Had a really quick look, but do the default values actually work?

@sanderPostma
Copy link
Contributor Author

Please change the delimiter to ||, or better yet to ?? So that is it more natural to read.
Fixed, using ?? both also supports ||

Had a really quick look, but do the default values actually work?
Tested all three states, also after the ?? change

@sanderPostma sanderPostma marked this pull request as ready for review November 15, 2023 15:20
@nklomp
Copy link
Contributor

nklomp commented Nov 15, 2023

The correlationIds map VCI option files onto VCI metadata and VP option files onto presentation_definitions files. So they need to be unique for a tuple of these files. For now a correlationId related to VCI does not affect VP or vice versa, so there can be overlap between these. However it is wise to just have a correlationId unique per ecosystem, which then is present in a single file for VCI options, VCI metadata, VP options and presentation_definition

Current changes to correlationId do not ensure that. We can/should just use another identifier for the them, like for instance the ecosystem name

@sanderPostma sanderPostma marked this pull request as draft November 17, 2023 16:05
@sanderPostma
Copy link
Contributor Author

We do not need this now. Let's see in sprint review/start what to do with this

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