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

Adds --follow command to odo log #496

Merged
merged 1 commit into from Jun 8, 2018
Merged

Conversation

cdrage
Copy link
Member

@cdrage cdrage commented May 31, 2018

This adds the --follow or -f command to odo log in order to stream logs
as they are being built for DeploymentConfig.

Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

If we are following logs, wouldn't it make more sense to just show new lines instead of showing everything from the start?
If I want to use odo log -f I'm more interested in the future log lines than in what was there before. For that, I can run just odo log.

cmd/log.go Outdated
checkError(err, "Unable to retrieve logs, does your component exist?")
},
}

func init() {
rootCmd.AddCommand(logCmd)

logCmd.Flags().BoolVarP(&logWatch, "watch", "w", false, "Stream logs")
Copy link
Member

Choose a reason for hiding this comment

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

can we use --follow / -f?
I think that -f is more standard for this kind of things. kubect logs -f, tail -f ...

@cdrage
Copy link
Member Author

cdrage commented Jun 1, 2018

@kadel I'll make the changes! -f or --follow is indeed a better choice. I was conflicted between --watch or --follow.

@cdrage cdrage force-pushed the add-watch branch 2 times, most recently from df786b4 to 59dc27a Compare June 1, 2018 14:13
@cdrage cdrage changed the title Adds --watch command to odo log Adds --follow command to odo log Jun 1, 2018
@cdrage cdrage force-pushed the add-watch branch 2 times, most recently from d7f7324 to 14b2bf9 Compare June 1, 2018 14:50
@cdrage cdrage added the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Jun 1, 2018
// TODO: https://github.com/kubernetes/kubernetes/pull/60696
// Unable to set to 0, until we update our vendoring
// set to 1 for now.
tailLines := int64(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Need your insight on this @kadel

I'm blocked on issue kubernetes/kubernetes#60696

The commit didn't make it into the 1.10.0 Kubernetes release freeze. It will however be in the 1.11.0 release.

At the moment, the client-go vendoring for OpenShift is at 1.10.2 https://github.com/openshift/client-go/blob/master/glide.yaml#L14

So we literally just have to wait until 1.11.0 of Kubernetes is released and go from there.

The current work-around is to set it to 1, where it will only output 1 line from the previous log, until 1.11.0 is released.

@cdrage
Copy link
Member Author

cdrage commented Jun 1, 2018

Need your insight on this @kadel

https://github.com/redhat-developer/odo/pull/496/files#diff-6025e9b620e9321c03c11bf29e5c4dbdR1068

	// If the log is being followed, set it to follow / don't wait
	if followLog {
		// TODO: https://github.com/kubernetes/kubernetes/pull/60696
		// Unable to set to 0, until openshift/client-go updates their Kubernetes vendoring to 1.11.0
		// Set to 1 for now.
		tailLines := int64(1)
		deploymentLogOptions = appsv1.DeploymentLogOptions{Follow: true, NoWait: false, Previous: false, TailLines: &tailLines}
	}

I'm blocked on issue kubernetes/kubernetes#60696

The commit didn't make it into the 1.10.0 Kubernetes release freeze. It will however be in the 1.11.0 release.

At the moment, the client-go vendoring for OpenShift is at 1.10.2 https://github.com/openshift/client-go/blob/master/glide.yaml#L14

So we literally just have to wait until 1.11.0 of Kubernetes is released and go from there.

The current work-around is to set it to 1, where it will only output 1 line from the previous log, until 1.11.0 is released.

IMO, we should just add the TODO and leave an issue open until it's fixed.

@kadel
Copy link
Member

kadel commented Jun 4, 2018

Setting it to 1 is OK. It might be even better than 0, the user sees something when the command is run.

It the future it might be good to have this as a parameter.

But the current implementation is good for now. 👍

@cdrage
Copy link
Member Author

cdrage commented Jun 4, 2018

@kadel Awesome 👍

Code / review look good to you?

@kadel kadel removed the status/blocked Denotes an issue or PR that is blocked on something (e.g., issue/PR in different repo) label Jun 5, 2018
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

LGTM

This adds the --follow or -f command to `odo log` in order to stream logs
as they are being built for DeploymentConfig.
@kadel kadel merged commit 79eb3a4 into redhat-developer:master Jun 8, 2018
@kadel kadel mentioned this pull request Jun 12, 2018
@cdrage cdrage deleted the add-watch branch July 3, 2018 16:08
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

2 participants