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: RemoveYAMLComments considering hashtag within strings as comment #302

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

varas
Copy link
Contributor

@varas varas commented Jan 29, 2021

RemoveYAMLComments considering hashtag within strings as comment.

Golang does not support negative lookaheads golang/go#18868
There's an alternative library https://github.com/dlclark/regexp2 but here we are sticking to stdlib.
For this reason it's required:

  • several regexes
  • several capture groups that will be discarded

We are not trimming whitespaces for simplicity.


This also logs the (previously shadowed) original error on config error.

@varas varas added the wip label Jan 29, 2021
@josemore josemore added this to In Progress in CAOS: Our Daily Bread Feb 1, 2021
@varas varas force-pushed the fix_strip-yaml-comments branch 2 times, most recently from f307b7c to b5572f1 Compare February 4, 2021 15:08
@varas varas removed the wip label Feb 4, 2021
@varas varas requested a review from a team February 4, 2021 15:11
@varas varas moved this from In Progress to Ready For Review in CAOS: Our Daily Bread Feb 4, 2021
@coveralls
Copy link

coveralls commented Feb 4, 2021

Pull Request Test Coverage Report for Build 539802970

  • 33 of 38 (86.84%) changed or added relevant lines in 2 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 58.138%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/integrations/legacy/registry.go 0 1 0.0%
pkg/config/envvar/envvar.go 33 37 89.19%
Files with Coverage Reduction New Missed Lines %
pkg/metrics/sampler/sampler_routine.go 2 94.74%
pkg/integrations/v4/logs/cfg_watcher.go 4 67.5%
Totals Coverage Status
Change from base Build 518394642: 0.03%
Covered Lines: 11456
Relevant Lines: 19705

💛 - Coveralls

pkg/config/envvar/envvar.go Outdated Show resolved Hide resolved
@varas varas requested a review from a team February 5, 2021 08:20
CAOS: Our Daily Bread automation moved this from Ready For Review to Ready For Release Feb 5, 2021
Copy link
Contributor

@noly noly left a comment

Choose a reason for hiding this comment

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

thanks to taking care of this!

@varas varas merged commit 292737e into master Feb 5, 2021
@varas varas deleted the fix_strip-yaml-comments branch February 5, 2021 09:31
@brushknight brushknight moved this from Ready For Release to Done in CAOS: Our Daily Bread Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants