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

env: fix a bunch of escaping/parsing edge cases #308

Merged
merged 6 commits into from Sep 26, 2022

Conversation

milas
Copy link
Member

@milas milas commented Sep 23, 2022

  • Handle standard escape sequences based on XSI/XBD https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html
  • Do not strip \ for non-escape sequences (e.g. \j should still be \j, not j)
  • Fix parsing with escaped quotes at end of quoted value
  • Handle key whose name starts with export (require whitespace between export keyword when stripping)
  • Fix parsing of unquoted values with embedded # (require a space before starting an inline comment)
  • Use correct parser methods throughout all test cases
  • Eliminate a lot of unused code that was not working right

@milas milas added the bug Something isn't working label Sep 23, 2022
@milas milas self-assigned this Sep 23, 2022
@milas milas requested a review from ndeloof as a code owner Sep 23, 2022
parseAndCompare(t, `FOO="bar\n\ b\az"`, "FOO", "bar\n\\ b\az")
parseAndCompare(t, `FOO="bar\\\n\ b\az"`, "FOO", "bar\\\n\\ b\az")
parseAndCompare(t, `FOO="bar\\r\ b\az"`, "FOO", "bar\\r\\ b\az")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.B. It was buggy here before! It was stripping \ characters on "unknown" escape sequences, which is...not desirable 🙈

* Handle standard escape sequences based on XSI/XBD
  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/echo.html
* Fix parsing with escaped quotes at end of quoted
  value
* Handle key whose name starts with `export` (require
  whitespace between `export` keyword when stripping)
* Fix parsing of unquoted values with embedded `#`
  (require a space before starting an inline comment)
* Use correct parser methods throughout all test cases
* Eliminate a lot of unused code that was not working
  right

Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice cleanup

dotenv/cut.go Outdated Show resolved Hide resolved
dotenv/godotenv.go Outdated Show resolved Hide resolved
dotenv/godotenv.go Outdated Show resolved Hide resolved
dotenv/godotenv_test.go Show resolved Hide resolved
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IHMO no need to support Golang 1.17 anymore and to avoid breaking changes in downstream repositories we should not remove exported methods.

dotenv/cut_go117.go Outdated Show resolved Hide resolved
dotenv/godotenv.go Outdated Show resolved Hide resolved
@milas milas requested a review from glours Sep 26, 2022
@@ -1,6 +1,6 @@
module github.com/compose-spec/compose-go

go 1.17
go 1.18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫣 Ho I didn't check we were still using 1.17 here 😬

glours
glours approved these changes Sep 26, 2022
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

milas added 4 commits Sep 26, 2022
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
@milas milas merged commit 08a21df into compose-spec:master Sep 26, 2022
8 checks passed
@milas milas deleted the env-parser-escapes branch Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants