skip warnings on commented out and empty lines

[sfenman]

Q	A
🐛 Bug fix?	no
🚀 New feature?	no
⚠ Deprecations?	no
❌ BC Break	no
🔗 Related issues	#157
❓ Documentation	no

Description

Created a condition to skip empty or commented out (starting with # lines) from checking.

[codecov]

Codecov Report

Merging #178 (d9d1d01) into main (4e486b3) will increase coverage by 0.03%.
The diff coverage is 8.67%.

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   69.68%   69.71%   +0.03%     
==========================================
  Files         190      190              
  Lines        4364     4379      +15     
==========================================
+ Hits         3041     3053      +12     
- Misses       1082     1086       +4     
+ Partials      241      240       -1     

Impacted Files	Coverage Δ
pkg/cmd/scan.go	79.04% <0.00%> (-0.96%)	⬇️
pkg/remote/aws/db_instance_supplier.go	66.66% <0.00%> (ø)
pkg/remote/aws/db_subnet_group_supplier.go	65.51% <0.00%> (ø)
pkg/remote/aws/ec2_ami_supplier.go	66.66% <0.00%> (ø)
pkg/remote/aws/ec2_ebs_snapshot_supplier.go	74.35% <0.00%> (ø)
pkg/remote/aws/ec2_ebs_volume_supplier.go	72.22% <0.00%> (ø)
pkg/remote/aws/ec2_eip_association_supplier.go	68.75% <0.00%> (ø)
pkg/remote/aws/ec2_eip_supplier.go	68.75% <0.00%> (ø)
pkg/remote/aws/ec2_instance_supplier.go	73.68% <0.00%> (ø)
pkg/remote/aws/ec2_key_pair_supplier.go	62.96% <0.00%> (ø)
... and 35 more

[eliecharra]

nbArgs == 0 is not exactly the check we want to ignore, we want to check if the line is empty

Your if directive should be moved just after line := scanner.Text().
We do not want to call readDriftIgnoreLine for ignored lines.

You can also remove the else directive as you put a continue.

Something like below should do the job :

line := scanner.Text()
+if line == "" || strings.HasPrefix(line, "#") {
+  logrus.WithFields(logrus.Fields{
+    "line": line,
+  }).Debug("Skipped comment or empty line")
+  continue
+}
typeVal := readDriftIgnoreLine(line)

[eliecharra]

Hi @sfenman thanks a lot for this PR 😃 , could you update the test suite to reflect your changes too ? 🙏🏻

[sfenman]

Hi @eliecharra, thank you for the comments, I will update it shortly. I guess a new testing function is needed to test about empty and commented out lines explicitly right?

[eliecharra]

Hi @eliecharra, thank you for the comments, I will update it shortly. I guess a new testing function is needed to test about empty and commented out lines explicitly right?

I think you just have to update drift_ignore_valid/.driftignore by adding empty lines and comment.

We do not want to test that no warning are send at the moment as it will imply to use and inject a custom logger in driftignore logic to be able to mock it. So don't spend too much time on this 😃

[sfenman]

I've made the changes based on your comments, but I couldn't omit the else directive because without it, the comments and the empty lines are still being passed to readDriftIgnoreLine .

[eliecharra]

I've made the changes based on your comments, but I couldn't omit the else directive because without it, the comments and the empty lines are still being passed to readDriftIgnoreLine .

You are missing the continue directive below in your if after the debug log, it will skip the line without the need for an else statement.

It will make the code more readable and avoid unnecessary else nesting.

[eliecharra]

Awesome, thanks a lot for this fix @sfenman 🙏🏻

[wbeuil]

@all-contributors please add @sfenman for code

[allcontributors]

@wbeuil

I've put up a pull request to add @sfenman! 🎉