Comment parsing and modifiers #535

New Issue

2023-11-14T14:21:51Z

p4u1 commented

2023-11-14 14:21:51 +00:00

Background
I'm currently investigating this failing test on the master branch:

=== RUN   TestEnvVarCommentsRemoved
    env_test.go:243: comment from 'foo # should be removed' should be removed

I think this failure got introduced by ccf0215495

Proposal
While trying to fix this issue, i was not sure what the desired behaviour actually was. Here is a proposal, of how comment parsing could work in config env files:

line	value	modifiers
# Foo	empty	none
#Foo	empty	none
Foo=bar # my comment	bar	none
Foo=bar#baz	bar#baz	none
Foo="# bar"	# bar	none
Foo="bar #"	bar #	none
Foo='bar #'	bar #	none
Foo=bar # one=two	bar	one=two
Foo=bar # length=10	bar	length=10
Foo=bar # length=10 width=20	bar	length=10, width=20

About a possible implementation
We currently use a fork (https://github.com/Autonomic-Cooperative/godotenv) of the godotenv library. In this fork inline comment removal got removed (https://github.com/x1unix/godotenv/commit/b031ea1211e7fd297af4c7747ffb562ebe00cd33), probably to be able to implement modifiers. To implement proper comment parsing and modifiers, I think its best to either implement this in the fork or inline the library to abra (I would prefer this)

**Background** I'm currently investigating this failing test on the master branch: ``` === RUN TestEnvVarCommentsRemoved env_test.go:243: comment from 'foo # should be removed' should be removed ``` I think this failure got introduced by https://git.coopcloud.tech/coop-cloud/abra/commit/ccf021549506f7b2f4112cfed81605ec4daa8a45 **Proposal** While trying to fix this issue, i was not sure what the desired behaviour actually was. Here is a proposal, of how comment parsing could work in config env files: | line | value | modifiers | | ---------------------------- | ------- | ------------------- | | # Foo | empty | none | | #Foo | empty | none | | Foo=bar # my comment | bar | none | | Foo=bar#baz | bar#baz | none | | Foo="# bar" | # bar | none | | Foo="bar #" | bar # | none | | Foo='bar #' | bar # | none | | Foo=bar # one=two | bar | one=two | | Foo=bar # length=10 | bar | length=10 | | Foo=bar # length=10 width=20 | bar | length=10, width=20 | **About a possible implementation** We currently use a fork (https://github.com/Autonomic-Cooperative/godotenv) of the godotenv library. In this fork inline comment removal got removed (https://github.com/x1unix/godotenv/commit/b031ea1211e7fd297af4c7747ffb562ebe00cd33), probably to be able to implement modifiers. To implement proper comment parsing and modifiers, I think its best to either implement this in the fork or inline the library to abra (I would prefer this)

knoflook commented

2023-11-23 12:18:34 +00:00

also reported in coop-cloud/organising#524

also reported in https://git.coopcloud.tech/coop-cloud/organising/issues/524

knoflook referenced this issue

2023-11-23 12:19:44 +00:00

Sometimes variables in .env contain # so the fix to remove end comment breaks them #524

knoflook added the bug abra labels 2023-11-23 12:34:59 +00:00

decentral1se added this to the (deleted) project 2023-11-28 09:05:20 +00:00

decentral1se referenced this issue

2023-11-28 09:08:39 +00:00

Consider caarlos0/env instead of forked godotenv? #459

decentral1se commented

2023-11-28 09:11:35 +00:00

Bumped up to "Critical fixes" and mentioned on the fedi channel. Think this needs some work to fix up ASAP.

Agree with your table listing @p4u1, that's the intended functionality as far as I understand.

I was looking at another library coop-cloud/organising#459 at some point for re-using instead of maintaining the fork. Whcih is now probably even broken? I'm not sure as that was definitely a hack on top of the comment parsing.

Hopefully someone can pick this up soon.

@knoflook thanks for the hotfix. i'd have preferred if you'd pinged us on the matrix channels or raised an issue that you're pushing it tho, would have saved the puzzling as to why the test suite is broken. for next time...

Bumped up to "Critical fixes" and mentioned on the fedi channel. Think this needs some work to fix up ASAP. Agree with your table listing @p4u1, that's the intended functionality as far as I understand. I was looking at another library https://git.coopcloud.tech/coop-cloud/organising/issues/459 at some point for re-using instead of maintaining the fork. Whcih is now probably even broken? I'm not sure as that was definitely a hack on top of the comment parsing. Hopefully someone can pick this up soon. @knoflook thanks for the hotfix. i'd have preferred if you'd pinged us on the matrix channels or raised an issue that you're pushing it tho, would have saved the puzzling as to why the test suite is broken. for next time...

decentral1se referenced this issue from a commit

2023-11-28 10:42:58 +00:00

test: skip failing test for now

p4u1 commented

2023-11-28 18:28:02 +00:00

I can work on this in the next days. Will have a look at the library you linked and will also look for some other libraries. But I'm pretty certain, that we need a custom library for this custom behavior.

🎉 1

decentral1se commented

2023-11-28 20:28:04 +00:00

Thanks!

I think actually this needs to be fixed before coop-cloud/organising#464 can be fixed. The hotfix is borking the ability of other code to strip or include modifiers. Which I think highlights the code smell that the modifiers are not parsed immediately at load time, they are passed in strings until a later time (config.ReadEnvOptions{...}) 😬

For #464, we need a single function that can read the recipe config (all compose files, see ReadSecretsConfig), parse modifiers etc. and give back a struct with the secret ID (the key in the compose secret config), secret name (all comments stripped) and secret version struct (with modifiers parsed).

Thanks! I think actually this needs to be fixed before https://git.coopcloud.tech/coop-cloud/organising/issues/464 can be fixed. The hotfix is borking the ability of other code to strip or include modifiers. Which I think highlights the code smell that the modifiers are not parsed immediately at load time, they are passed in strings until a later time (`config.ReadEnvOptions{...}`) 😬 For #464, we need a single function that can read the recipe config (all compose files, see `ReadSecretsConfig`), parse modifiers etc. and give back a struct with the secret ID (the key in the compose secret config), secret name (all comments stripped) and secret version struct (with modifiers parsed).

decentral1se referenced this issue

2023-11-28 20:28:33 +00:00

Secret name generation only reads env vars #464

p4u1 referenced this issue from toolshed/abra

2023-11-30 07:28:12 +00:00

proper env modifiers support #391

p4u1 referenced this issue from a commit

2023-12-01 09:15:50 +00:00

proper env modifiers support

decentral1se referenced this issue from a commit

2023-12-01 11:03:55 +00:00

proper env modifiers support