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

implement var substitution feature #30

Merged
merged 6 commits into from
May 8, 2023

Conversation

mikerott
Copy link
Contributor

@mikerott mikerott commented Apr 28, 2023

The README already implied that the feature this PR introduces was a thing. It was not, but after this PR, it is! Consider this PR a proposal for a feature, and because I did not add feature-specific function names to the SDK, this PR represents a breaking change and will require a major version update. That said, I expect that no one will actually be broken upon updating.

Below was my original review request and is now outdated.

All backward compatibility remains intact, so this will be a minor version update.

I have a specific review question. This PR, as it is, behaves such that if a substitution is desired, but implemented incorrectly, the result is to return nil, false (nil value, not found) rather than the unsubstituted value.

For example, imagine the env vars are:

A=aValue

and the YAML file is:

B: "${C}"

The SDK could behave such that "find B" returns nil, false or it returns "${C}", true. I think I prefer the former, as it's the earliest warning sign to the user of this lib that they're seeking a key (C) that does not exist.

@mikerott mikerott requested a review from a team as a code owner April 28, 2023 14:39
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #30 (9b015aa) into master (604a53e) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   91.27%   91.60%   +0.32%     
==========================================
  Files           6        6              
  Lines         871      905      +34     
==========================================
+ Hits          795      829      +34     
  Misses         62       62              
  Partials       14       14              
Impacted Files Coverage Δ
source.go 76.81% <100.00%> (+7.58%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mikerott mikerott force-pushed the feature/var-substitution branch from 132f813 to 9b9b7fd Compare April 28, 2023 14:46
@mikerott mikerott force-pushed the feature/var-substitution branch from 9b9b7fd to f008036 Compare April 28, 2023 15:00
@mikerott mikerott changed the title implement var substituion feature implement var substitution feature Apr 28, 2023
README.md Outdated Show resolved Hide resolved
source.go Outdated Show resolved Hide resolved
source_test.go Show resolved Hide resolved
source.go Outdated Show resolved Hide resolved
source.go Outdated Show resolved Hide resolved
@dontfollowsean dontfollowsean merged commit 235ff72 into master May 8, 2023
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.

4 participants