Skip to content

Fix WithConfig instances #407

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

Merged
merged 2 commits into from
Nov 7, 2018
Merged

Conversation

CapacitorSet
Copy link
Contributor

@CapacitorSet CapacitorSet commented Aug 1, 2018

Discovered while fixing #402. Not yet ready for merging, I'm waiting for feedback on the issue.

@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@448bc32). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #407   +/-   ##
=========================================
  Coverage          ?   41.69%           
=========================================
  Files             ?       78           
  Lines             ?     8792           
  Branches          ?        0           
=========================================
  Hits              ?     3666           
  Misses            ?     4701           
  Partials          ?      425
Impacted Files Coverage Δ
server/honeytrap.go 3.19% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 448bc32...b0bccce. Read the comment docs.

@nl5887
Copy link
Contributor

nl5887 commented Aug 1, 2018

Waiting for feedback if issue has been resolved?

@nl5887
Copy link
Contributor

nl5887 commented Aug 1, 2018

@sammynx could you verify?

@CapacitorSet
Copy link
Contributor Author

Yes, I'm waiting for confirmation that they're actually unused (so that I know it's not a bug in WithConfig).

@sammynx
Copy link
Collaborator

sammynx commented Aug 1, 2018

The warning is correct, the values are unused. This is the issue. However when I tested this pull, the warning is gone for me. So it's hiding a problem now.

@CapacitorSet
Copy link
Contributor Author

That's weird, this doesn't happen for me with a simple config (single port, single service, users="123", host="456"). Can you paste your configuration?

@sammynx
Copy link
Collaborator

sammynx commented Aug 9, 2018

I don't know how my config looked anymore, because I changed it a lot recently. I was working on this issue with smtp. There must have been a mixup with configs, because I can't recreate my issues from last comment.

Situation now:
on a build from master I get this warning
honeytrap/server ▶ WARN 00a Unrecognized keys in configuration: [channel.file.filename]
However it is using channel.file.filename

on a build from this PR I get no warnings.

So this is working for me now. Sorry for the confusion I made in earlier comments.

This is the config I used for this.

[listener]
type="socket"

[service.smtp]
type="smtp"
#host="smtp.mailer.org"
#name="HT SMTP-E v2.3.0.1b"
#banner-fmt='{{.Host}} {{.Name}} - Ready'
#users=[ ["root","root"], ["admin", "admin"] ]
#secrets=[ ["user", "password"] ]

[[port]]
port="tcp/8025"
services=["smtp"]

[channel.console]
type="console"

[channel.file]
type="file"
filename="/tmp/honeytrap.log"

[[filter]]
channel=["console", "file"]

[[logging]]
output = "stdout"
level = "debug"

@CapacitorSet
Copy link
Contributor Author

Thank you for your feedback.

This PR is ready for merging, then.

@CapacitorSet CapacitorSet changed the title [WIP] Fix WithConfig instances Fix WithConfig instances Aug 9, 2018
@nl5887 nl5887 merged commit b0bccce into honeytrap:master Nov 7, 2018
@nl5887
Copy link
Contributor

nl5887 commented Nov 7, 2018

Merged, thanks!

@CapacitorSet CapacitorSet deleted the fix-withconfig branch November 7, 2018 14:06
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.

4 participants