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

fix order issue for env expand #11

Closed
wants to merge 1 commit into from
Closed

Conversation

ysmood
Copy link

@ysmood ysmood commented Dec 21, 2023

For example if we have a .env file like:

A=1
B=${A}
C=${B}

When we want to iterate items in the go map the order will be random. There's no way we can guarantee the .env can be properly expanded.

The expect outcome:

A=1
B=1
C=1

But we may get:

A=1
B=""
C=""

For example if we have a .env file like:

```bash
A=1
B=${A}
C=${B}
```

When we want to iterate items in the go map the order will be random.
There's no way we can guarantee the .env can be properly expanded.
@hashicorp-cla
Copy link

hashicorp-cla commented Dec 21, 2023

CLA assistant check
All committers have signed the CLA.

@ysmood
Copy link
Author

ysmood commented Dec 22, 2023

@schmichael Could you help?

@schmichael schmichael self-assigned this Jan 3, 2024
schmichael added a commit that referenced this pull request Jan 3, 2024
Inspired by #11 but altered for a few reasons:

1. I didn't want to change the API
2. When deduplicating keys #11 chose to keep the first position the key
   occurred at while I chose to keep the last so that the position of
   the key corresponds to the position of its value.
3. I added some benchmarks to ensure the technically quadratic ordered
   implementation wasn't pathologically slower than the linear
   implementation.

The benchmarks show the ordered implementation to be faster for my
synthesized "little" and "big" datasets:

```
BenchmarkLittle/Parse-20                  284787              3960 ns/op
BenchmarkLittle/ParsePairs-20             320619              3555 ns/op
BenchmarkBig/Parse-20                          1        1996228538 ns/op
BenchmarkBig/ParsePairs-20                     1        1900769619 ns/op
```
@schmichael
Copy link
Member

Hi @ysmood and thanks for the submission! Sorry for the delayed response, but I was on holiday vacation.

Since go-envparse does not perform interpolation so this isn't a bug. I've labeled it an enhancement since without an order preserving API it is very difficult to resolve dependencies in your example.

Would you mind taking a look at #12 and letting me know what you think? It's based on your changes but refactored to preserve the existing API and expose a lower level iterator API.

@ysmood
Copy link
Author

ysmood commented Jan 4, 2024

@schmichael Thanks, approved!

@ysmood ysmood closed this Jan 4, 2024
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.

3 participants