-
Notifications
You must be signed in to change notification settings - Fork 17
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
Feature/env variables #127
Draft
SvenLieber
wants to merge
3
commits into
RMLio:development
Choose a base branch
from
SvenLieber:feature/env-variables
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use normal brackets as well here? And check all variables if they are defined in the
.env
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean
$(DB_PASSWORD)
?This would clash with YARRRML templates: https://rml.io/yarrrml/spec/#template
Thus if you would (for some reason) specify
value=myValue
in your.env
then also yarrrml templates$(value)
would be replaced.Similarly, if you would define
VALUE=myValue
in.env
then a potential yarrrml template$(VALUE)
would be replaced.Such a clash with YARRRML templates is in my opinion not intended, because they reflect e.g. column names in a source.
Furthermore, curly braces (or no braces) are the common way to represent (environment) variables in UNIX, regular brackets are used for sub-shells: https://dev.to/rpalo/bash-brackets-quick-reference-4eh6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or do the same as with the external references? Adding a
_
before the variable name? so you would have then$(_DB_PASSWORD)
? So external references can come from both the CLI and.env
, which makes sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would technically work but would be a convention which has to be used just because of YARRRML and therefore would harm reusability/interoperability in cases where YARRRML is not the only component.
There might be other components using
.env
variables, for example a script which creates an SSH tunnel to a remote DB host behind a firewall, some UI component or any other application doing something.IMHO YARRRML should integrate into an existing setup as easy as possible. A setup in which certain environment variables are already in place and are used by other systems. As a user I would not want to change my whole application the moment I want to integrate YARRRML mappings. This could be a potential pitfall preventing people from using YARRRML.
As I see it, reducing the expressibility of environment variables (only with prefixed underscore) just to combine it with external references (which already can be provided via parameter) would limit this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only have to change your YARRRML files, nothing else. The variables names remain the same. You only add the
_
in the YARRRML template.