Skip to content
This repository was archived by the owner on Feb 5, 2020. It is now read-only.

kube_version: update for installer integration#13

Merged
squeed merged 2 commits intocoreos:masterfrom
lucab:ups/kube-version
Aug 11, 2017
Merged

kube_version: update for installer integration#13
squeed merged 2 commits intocoreos:masterfrom
lucab:ups/kube-version

Conversation

@lucab
Copy link
Contributor

@lucab lucab commented Aug 10, 2017

No description provided.

@lucab lucab requested a review from squeed August 10, 2017 13:44
return nil
}

// GetKubeVersion retrieves kubernetes version querying several sources
Copy link

Choose a reason for hiding this comment

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

Can you list the sources here?

)

// WriteKubeletEnv writes the `kubelet.env` file
func (a *App) WriteKubeletEnv(destPath string) error {
Copy link

Choose a reason for hiding this comment

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

This needs to write the kubernetes version from the API if available.

defer dstFp.Close()

for k, v := range flags {
if k == envVersionKey && kubeletVersion != "" {
Copy link

Choose a reason for hiding this comment

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

Do you want to add the versionKey if its not present in the file?

Copy link
Contributor Author

@lucab lucab Aug 11, 2017

Choose a reason for hiding this comment

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

Mmh, an emergency fallback would not hurt.

After some more thinking, I prefer to let the kubelet service fail to start in such cases. Otherwise debugging what's wrong here can be quite hard.

Copy link

Choose a reason for hiding this comment

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

Ah, that's reasonable.

@squeed
Copy link

squeed commented Aug 11, 2017

One small question, otherwise LGTM. Feel free to merge.

@lucab
Copy link
Contributor Author

lucab commented Aug 11, 2017

coreos/tectonic-installer#1659 is a pre-requisite for this.

@squeed squeed merged commit 2b64a69 into coreos:master Aug 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants