Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add extra hosts option #755

Merged
merged 2 commits into from
Jan 6, 2023
Merged

Conversation

Haegi
Copy link
Contributor

@Haegi Haegi commented Dec 20, 2022

Context

At the moment gnomock doesn't support adding extra hosts to the container.
This PR adds a WithExtraHosts option, which implements exactly that.
If the option is provided, the extra hosts will be added to the container and otherwise it will just receive an empty array and it will do nothing with it.

Notes

I am not sure on a proper test for it.
Maybe I could run some busybox and check if the specified host is reachable.

Tests:

  • ping with extra host (success)
  • ping without extra host (failure)
  • ping with extra host to other domain (success)

@orlangure
Copy link
Owner

Hi @Haegi and thank you for the contribution! I wasn't aware of this flag, this is really cool!
I like your suggested test scenario. Can you update the PR with it?
Thanks!😻

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Base: 86.09% // Head: 65.71% // Decreases project coverage by -20.37% ⚠️

Coverage data is based on head (b26ec39) compared to base (e685fca).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #755       +/-   ##
===========================================
- Coverage   86.09%   65.71%   -20.38%     
===========================================
  Files          49       49               
  Lines        2315     2319        +4     
===========================================
- Hits         1993     1524      -469     
- Misses        167      687      +520     
+ Partials      155      108       -47     
Impacted Files Coverage Δ
docker.go 91.34% <100.00%> (+0.90%) ⬆️
options.go 100.00% <100.00%> (ø)
preset/kafka/options.go 0.00% <0.00%> (-100.00%) ⬇️
preset/splunk/options.go 0.00% <0.00%> (-100.00%) ⬇️
preset/cassandra/options.go 0.00% <0.00%> (-100.00%) ⬇️
preset/localstack/options.go 0.00% <0.00%> (-100.00%) ⬇️
preset/splunk/preset.go 1.66% <0.00%> (-95.00%) ⬇️
preset/localstack/preset.go 1.16% <0.00%> (-91.87%) ⬇️
preset/cassandra/preset.go 3.44% <0.00%> (-89.66%) ⬇️
preset/kafka/preset.go 0.74% <0.00%> (-78.36%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Haegi
Copy link
Contributor Author

Haegi commented Dec 21, 2022

Hi @Haegi and thank you for the contribution! I wasn't aware of this flag, this is really cool! I like your suggested test scenario. Can you update the PR with it? Thanks!😻

I just added some tests and I also adapted the PR description.
However somehow the k3s tests fail (also locally without the change in my case). Do you have any idea about it or should I have a deeper look at it?

@orlangure
Copy link
Owner

I'll take a look, I guess something broke in k3s preset😿

@Haegi
Copy link
Contributor Author

Haegi commented Dec 21, 2022

I'll take a look, I guess something broke in k3s preset😿

Thanks :)

@orlangure
Copy link
Owner

Hey, I fixed the k3s preset issue in #760, please rebase from master, the build should be OK😼

@Haegi Haegi force-pushed the bh-add-extra-hosts branch from 488e545 to b26ec39 Compare December 27, 2022 13:01
@Haegi
Copy link
Contributor Author

Haegi commented Dec 28, 2022

Hey, I fixed the k3s preset issue in #760, please rebase from master, the build should be OK😼

Thanks @orlangure! I rebased from master but it seems like the github action is now broken. Is this because of my change and should I have a look at it?

@orlangure
Copy link
Owner

@Haegi, no this is just a flaky job that I don't have capacity to look into. I will merge and prepare a release this weekend, everything looks great. Thank you!

@orlangure orlangure merged commit 0cb0065 into orlangure:master Jan 6, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants