Add flags on logs

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.

[seguins]

This PR adds the --follow, --timestamps and --tail options on logs (#2227).

[dnephin]

Instead of passing these as individual properties, could we do it as a single dict log_args ?

[dnephin]

Thanks for this PR! This is looking good, although I'm not sure if we can just change the api call from attach to logs. We'll have to make sure we have reasonable test coverage in this area, and try it out manually.

The test failure is unrelated I think.

[seguins]

I transformed the logs args as a dictionary.

I added some tests, mainly acceptance. I'm sure you would like integration or units tests, but I don't know how to do that because it's mainly api calls ?

[j-san]

+1 for tail

[dnephin]

Small problem here, we should show an error if --tail is invalid instead of just ignoring it.

image_type_from_opt() is a good example for this. It lets you move this if branch into the validation function as well.

[dnephin]

Thanks! This is looking great. Couple comments around error reporting and testing.

[dnephin]

stream and follow should be different things, however it looks like they were set to the same thing in docker-py. stream should say "return an iterator", where as follow is a behaviour of the API to continue to send logs or stop once it receives the latest one.

I think we need to fix docker-py to have separate params, then this should be:

logs_args['follow'] = ...

stream should always be True.

[seguins]

I understand the difference between stream and follow, but I'm not sure to understand the advantage of this change.

If you set follow to True, should stream always be True ?

Maybe should we open an issue on docker-py to talk about it before to merge this PR ?

[dnephin]

Yes, I think follow=True does imply stream=True, but stream=True doesn't imply follow=True, you may still want to stream without following.

Yes, I created #934

[seguins]

Thanks for the feedback, I added an error message when the tail parameter is invalid, and I rewrote some tests.

I'm waiting for the issue docker/docker-py#934 to finish this PR.

[SvenDowideit]

the documentation checker failure is due to the tutorials repo going private

#2928 will solve that when merged.

[dnephin]

Cool, the docker-py change has been merged and will be in the next release.

For now you can use a git url and git sha for the docker-py dependency in requirements.txt: For example: git+https://github.com/docker/docker-py.git@81d8caaf36159bf1accd86eab2e157bf8dd071a9#egg=docker-py

[seguins]

I changed the docker-py dependency. Now, we always call the logs api with stream=True.

[dnephin]

Using a mutable value as a default kwarg value can lead to a lot of confusion.

Instead of this default, please use

def__init__(self, ..., log_args=None):
log_args=log_argsor {}
...

[dnephin]

This is looking good. I'm a little surprised there isn't any changes to def up(). I suspect that it's going to need to pass in follow=True, and it's a bit concerning we don't have any tests failing without it.

[aanand]

Minor thing, but this could be a single assignment with a dictionary literal.

log_args= {
'follow': options['--follow'],
'timestamps': options['--timestamps'],
}

[seguins]

Thank you for the review. I applied fixes. I removed the part with threading and I rewrite the test_logs_follow test. I'm not sure it's the best way to test this case, but I haven't found better solution.

There is no change on up because it uses the attach api instead of logapi.

[dnephin]

There is no change on up because it uses the attach api instead of log api.

up uses LogPrinter, which used to attach, but is now using logs, so I think it's no longer using attach.

[seguins]

Indeed, I missed it ... Because docker-py has a backward compatibly, up still work.

Should we set explicitly follow=True ?

[dnephin]

I think it would be good to set that explicitly, yes

[seguins]

up uses a convergence_plan. Convergence plan sets should_attach_logs = not detached which has for effect to call attach_log_stream method of container. This method attachs the container and stores the result in log_stream.

LogPrinter checks if log_stream is set and in this case it's not call the logs api. up will not call the log api but the attach api.

However, we need to set follow=True because it's used for displaying the end of process.

Do you think there are enough tests if I just add to the test_up_attached test : assert 'exited with code 0' in stdout ?

[dnephin]

I see, you're right it does use attach, I forgot about that. I think it's fine as is.

[seguins]

I added the follow=True for up. This PR is ready for a new review.

[dnephin]

LGTM

[shin-]

Might be a good idea to rerun the tests on janky? Code looks good overall.

[aanand]

LGTM

[jayhding]

@aanand would you mind to comment if this merge will be done also to libcompose, which is used by rancher-compose? Thanks.

[aanand]

@JayHaoDing It will have to be separately implemented. libcompose is not high priority at the moment - we have a Compose release very soon, so that's what we're focusing on.