Skip to content

Env var support in agent configs#89

Draft
sanderPostma wants to merge 3 commits intofeat/VDX-308from
feat/VDX-308-env-subst
Draft

Env var support in agent configs#89
sanderPostma wants to merge 3 commits intofeat/VDX-308from
feat/VDX-308-env-subst

Conversation

@sanderPostma
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@zoemaas zoemaas force-pushed the feat/VDX-308-env-subst branch from 5fa8b0a to 81a005a Compare November 17, 2023 14:38
@sanderPostma sanderPostma marked this pull request as draft November 17, 2023 16:05
@sanderPostma
Copy link
Copy Markdown
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