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

cmd: crictl: add a config command #194

Merged
merged 1 commit into from Nov 19, 2017

Conversation

runcom
Copy link
Contributor

@runcom runcom commented Nov 15, 2017

This is a fairly simple patch that just adds a config command to get and set configurations similar to git config.
This would be very needed for higher level commands like kubeadm in order to gather stuff like the runtime-endpoint to query.

@feiskyer @Random-Liu PTAL

$ crictl config --get runtime-endpoint
/var/run/crio.sock

$ crictl config runtime-endpoint /var/run/dockershim.sock

$ crictl config --get runtime-endpoint
/var/run/dockershim.sock

$ cat /etc/crictl.yaml
runtime-endpoint: /var/run/dockershim.sock

Signed-off-by: Antonio Murdaca runcom@redhat.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 15, 2017
@runcom
Copy link
Contributor Author

runcom commented Nov 16, 2017

Bump

@Random-Liu Random-Liu self-assigned this Nov 16, 2017
@Random-Liu
Copy link
Contributor

@runcom Thanks! This is really useful!

configFile := context.GlobalString("config")
if _, err := os.Stat(configFile); err != nil {
// can't continue w/o config file
logrus.Fatalf("Falied to load config file: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create an empty config here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// Get config from file.
config, err := ReadConfig(configFile)
if err != nil {
logrus.Fatalf("Falied to load config file: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return error instead? And same for the other log.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We can create the default config file /etc/crictl.yaml if it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Random-Liu
Copy link
Contributor

LGTM other than the comments.

@feiskyer
Copy link
Member

@runcom Thanks for adding this.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Contributor Author

runcom commented Nov 17, 2017

Comments addressed guys

@feiskyer feiskyer added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2017
@feiskyer feiskyer merged commit e7a6236 into kubernetes-sigs:master Nov 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants