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

Specify port in 'Test_isValidResolvConf' #10532

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b-long
Copy link

@b-long b-long commented Jul 16, 2024

Proposed Changes

This PR expands testing of the 'Valid ResolvConf' scenario (pkg/agent/config/config_internal_test.go).

Types of Changes

New Feature

*I was not able to run tests locally, so I'm opening a pull request for the explicit purpose of running tests in CI. I'd like to know whether or not port numbers are supported, when installing with the --resolv-conf flag.

In other words, where an end-user might run a DNS server on an alternate port, and provide that number in a configuration file (e.g./etc/rancher/k3s/resolv.conf ) when installing like the following:

curl -sfL https://get.k3s.io | sh -s - --write-kubeconfig-mode 644 --resolv-conf /etc/rancher/k3s/resolv.conf

Verification

If CI passes, I expect that to be enough verification.

Testing

Yes.

Linked Issues

N/A

User-Facing Change

NONE

Further Comments

@b-long b-long requested a review from a team as a code owner July 16, 2024 14:56
@b-long b-long force-pushed the feature/test-resolv-with-port branch from 1670df9 to 0b237fc Compare July 16, 2024 14:58
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.33%. Comparing base (37830fe) to head (0b237fc).
Report is 5 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (37830fe) and HEAD (0b237fc). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (37830fe) HEAD (0b237fc)
e2etests 9 6
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10532      +/-   ##
==========================================
- Coverage   49.66%   43.33%   -6.33%     
==========================================
  Files         179      179              
  Lines       14936    14936              
==========================================
- Hits         7418     6473     -945     
- Misses       6158     7265    +1107     
+ Partials     1360     1198     -162     
Flag Coverage Δ
e2etests 36.32% <ø> (-10.26%) ⬇️
inttests 19.72% <ø> (ø)
unittests 13.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@b-long b-long marked this pull request as draft July 25, 2024 17:46
@b-long
Copy link
Author

b-long commented Jul 25, 2024

I just want to mention to the maintainers, as I understand it from my own local testing, running a DNS service on port 5300 and specifying it in /etc/rancher/k3s/resolv.conf was not respected.

Can anyone confirm?

That said, I do not recommend this PR to be merged since it seems the tests passing are a false positive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant