Skip to content

Update minikube context to detect outdated /etc/hosts file #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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 14, 2020
Merged

Update minikube context to detect outdated /etc/hosts file #12

merged 4 commits into from
Sep 14, 2020

Conversation

nicholastmosher
Copy link
Contributor

This is related to infinyon/fluvio#241. This encapsulates the configurations and logic needed to update the "dns context" of minikube (assigning minikubeCA).

A few points to note:

  • This is technically a breaking change. However, in all of our repos, the only usage seemed to be in fluvio cluster set-minikube-context, which I am updating to match this change.
  • This uses minikube profile list -o json in order to detect the current IP and port of minikube
  • This decouples updating the kubectl context from updating the /etc/hosts file and minimizes shell code
    • While we're here making changes, do we want to just write Rust to edit /etc/hosts rather than shelling out to sed?

sehz
sehz requested changes Sep 11, 2020
View reviewed changes
src/k8-config/src/context.rs
@@ -25,71 +35,234 @@ fn load_cert_auth() -> String {
.to_string()
}

pub struct Option {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this break semver requirements. we should keep existing API. it can be mapped new one but old code should work.

src/k8-config/src/context.rs

/// create kube context that copy current cluster configuration
pub fn create_dns_context(option: Option) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep existing API for compatibility. map to new one

@nicholastmosher
Copy link
Contributor Author

Alright, so what I did here was I moved all of the existing code into context::v1 and added #[deprecated] tags to all of the items. I added a re-export for pub use v1::* to the context module so all of those items may be imported in the same way from old clients. Then, I added the new MinikubeContext struct and new logic into the context module. I also updated the k8-ctx-util binary to use the new MinikubeContext struct.

You can test this out by checking out the minikube branch on the fluvio repo and running cargo run --bin fluvio -- cluster set-minikube-context. This will do the following:

  • Check whether the minikubeCA entry in /etc/hosts matches the current IP address of minikube as reported by minikube profile list -o json.
  • If the IP address is different, it will update /etc/hosts to match the new IP address. Otherwise, it will not touch /etc/hosts.
  • Either way, it will update the kubectl context.

@nicholastmosher
Copy link
Contributor Author

Not sure why CI is failing, unit tests work fine on my machine. Failure seems to be related to installing nightly, I wonder if the MacOS nightly rust build is just broken. I wonder if we should mark nightly CI builds as not required for CI to pass.

@nicholastmosher nicholastmosher mentioned this pull request Sep 12, 2020
Closed
@sehz
Copy link
Collaborator

sehz commented Sep 12, 2020

Similar failure happening on MacOS. Yes probably should disable nightly for now

sehz
sehz approved these changes Sep 12, 2020
View reviewed changes
Copy link
Collaborator

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@sehz
Copy link
Collaborator

sehz commented Sep 12, 2020

Can you remove nightly from CI? Rust tool chain has problem

@sehz sehz mentioned this pull request Sep 12, 2020
Closed
@sehz
Copy link
Collaborator

sehz commented Sep 12, 2020

rust-lang/rust#76638

sehz
sehz reviewed Sep 14, 2020
View reviewed changes
sehz
sehz approved these changes Sep 14, 2020
View reviewed changes
src/k8-config/Cargo.toml
@@ -15,5 +15,6 @@ tracing = "0.1.19"
dirs = "2.0.2"
serde = { version ="1.0.103", features = ['derive'] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put minikube context as optional feature. put tera and hostfile under that feature set

@sehz sehz merged commit 9290f5b into infinyon:master Sep 14, 2020
@nicholastmosher nicholastmosher deleted the context-updates branch September 14, 2020 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sehz sehz sehz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nicholastmosher @sehz