-
Notifications
You must be signed in to change notification settings - Fork 12
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
Prepare shared code: Support running on non default dns settings #893
Prepare shared code: Support running on non default dns settings #893
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review.
Would you mind if I added a bunch of commits to this PR to adjust a few things?
Just pushed my test fix, did not look at your changes. Dont plan on pushing more today (do a little testing) so feel free to add :) |
There is still some bits and pieces we want to change, mostly regarding the error type / error handling. This commit also moves the input fixtures for testing the resolv parser into dedicated files and utilizies rstest's #[files] attribute to generate tests based on these files. Co-authored-by: Nick Larsen <[email protected]>
Co-authored-by: Techassi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
Co-authored-by: Sebastian Bernauer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks, looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions about error handling. I'm happy to make the changes if you want?
Co-authored-by: Malte Sander <[email protected]> Co-authored-by: Techassi <[email protected]>
Co-authored-by: Malte Sander <[email protected]> Co-authored-by: Techassi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
let _ = KUBERNETES_CLUSTER_DOMAIN | ||
.set(retrieve_cluster_domain().context(ResolveKubernetesClusterDomainSnafu)?); | ||
create_client(field_manager).await | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a global?
|
||
match env::var(KUBERNETES_SERVICE_HOST_ENV) { | ||
Ok(_) => { | ||
let cluster_domain = retrieve_cluster_domain_from_resolv_conf(RESOLVE_CONF_FILE_PATH)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do this hack (and I still don't think we should), can we please at least punt that until vNext instead of sneaking it in just before the release?
Description
Part of stackabletech/issues#436.
Definition of Done Checklist
Author
Reviewer
Acceptance