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.

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

#669 - feat(services) - add service logs #671

Merged
merged 9 commits into from
Jul 8, 2017
Merged

#669 - feat(services) - add service logs #671

merged 9 commits into from
Jul 8, 2017

Conversation

WTFKr0
Copy link
Contributor

@WTFKr0 WTFKr0 commented Mar 14, 2017 •
edited
Loading

Closes #669
Need to wait for service logs Generally Available,17.06 / API v1.30

@deviantony
Copy link
Member

Service logs is still experimental, this won't be merge in Portainer until it's out of experimental state.

FYI, you can use the endpoint.apiVersion property available via the StateManager to get the version of the current endpoint.

@WTFKr0
Copy link
Contributor Author

WTFKr0 commented Apr 13, 2017

Still experimental in 17.04, waiting next release !

@WTFKr0
Copy link
Contributor Author

WTFKr0 commented May 4, 2017

This will be available in 17.05 :
moby/moby#32462
https://github.com/moby/moby/blob/17.05.x/CHANGELOG.md

@hason hason mentioned this pull request May 14, 2017
Closed
8 tasks
@deviantony deviantony added work-in-progress Indicates that the PR is a work-in-progress and not ready yet and removed status/pending labels May 19, 2017
@WTFKr0
Copy link
Contributor Author

WTFKr0 commented May 24, 2017

I just rebase and test, I'm facing this issue
moby/moby#33006
I think we need to wait stable 17.06 version to implement this feature

@deviantony deviantony mentioned this pull request May 26, 2017
Closed
@deviantony
Copy link
Member

Now that 17.06 is out, service logs seems to be stable. Could you rebase? Is this still in WIP status? Or should I give it a try?

@WTFKr0
Copy link
Contributor Author

WTFKr0 commented Jul 2, 2017

I rebase and test soon, and remove wip if ok

@deviantony deviantony added the rebase-required Indicates that the PR must be rebased on the latest development branch label Jul 3, 2017
@WTFKr0
Copy link
Contributor Author

WTFKr0 commented Jul 3, 2017 •
edited
Loading

Okay seems to work well
I let the WIP status for the 17.06 check
I think the pretty print with node will be part of future feature
You can give it a try if you are on 17.06 node

@deviantony deviantony removed rebase-required Indicates that the PR must be rebased on the latest development branch work-in-progress Indicates that the PR is a work-in-progress and not ready yet labels Jul 4, 2017
deviantony
deviantony requested changes Jul 4, 2017
View reviewed changes
Copy link
Member

@deviantony deviantony left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM, just need to display logs if API version >= 1.30.

Check my comments for minor changes / questions too.

Forget the pretty print for now.

app/components/serviceLogs/serviceLogsController.js Outdated
});
}

// initial call
Copy link
Member

Choose a reason for hiding this comment

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

Could you place this code in an initView() function ?

app/components/serviceLogs/serviceLogsController.js
}, function (data, status, headers, config) {
// Replace carriage returns with newlines to clean up output
data = data.replace(/[\r]/g, '\n');
// Strip 8 byte header from each line of output
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with the header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 8 bytes due to stream protocol, really no information on that but I found that https://docs.docker.com/engine/api/v1.30/#operation/ContainerAttach

app/components/serviceLogs/serviceLogsController.js Outdated
// Strip 8 byte header from each line of output
data = data.substring(8);
data = data.replace(/\n(.{8})/g, '\n');
// Delete 156 Chars // com.docker.swarm.node.id=sov730ei4f0s26940rdmcgeql,com.docker.swarm.service.id=bobi0ougfe8fg6ewxvmft7ph2,com.docker.swarm.task.id=vx1oxavbdz0654dbp8a70dxmx
Copy link
Member

Choose a reason for hiding this comment

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

Still required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No in my firsts tests, logs send node/service/tack id (156 chars) but in api v1.30, nothing, so I'll delete those comments

app/components/serviceLogs/serviceLogsController.js
}, function (data, status, headers, config) {
// Replace carriage returns with newlines to clean up output
data = data.replace(/[\r]/g, '\n');
// Strip 8 byte header from each line of output
Copy link
Member

Choose a reason for hiding this comment

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

See my comments below.

app/components/serviceLogs/serviceLogsController.js Outdated
// Strip 8 byte header from each line of output
data = data.substring(8);
data = data.replace(/\n(.{8})/g, '\n');
// Delete 156 Chars // com.docker.swarm.node.id=sov730ei4f0s26940rdmcgeql,com.docker.swarm.service.id=bobi0ougfe8fg6ewxvmft7ph2,com.docker.swarm.task.id=vx1oxavbdz0654dbp8a70dxmx
Copy link
Member

Choose a reason for hiding this comment

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

See my comments below.

app/components/serviceLogs/serviceLogsController.js Outdated
$scope.tailLines = 2000;

$('#loadingViewSpinner').show();
Service.get({id: $stateParams.id}, function (d) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you relocate this into the initView function?

app/components/serviceLogs/servicelogs.html Outdated
<rd-widget>
<rd-widget-body>
<div class="widget-icon grey pull-left">
<i class="fa fa-server"></i>
Copy link
Member

Choose a reason for hiding this comment

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

Please use fa-list-alt here.

app/components/service/service.html Outdated
@@ -72,6 +72,13 @@
<input type="text" class="form-control" ng-model="service.Image" ng-change="updateServiceAttribute(service, 'Image')" ng-disabled="isUpdating" />
</td>
</tr>
<tr>
Copy link
Member

Choose a reason for hiding this comment

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

We need to add a condition on this, only display this section for API version >= 1.30

@deviantony deviantony added the changes-required Waiting for an update of the contributor label Jul 4, 2017
@WTFKr0 WTFKr0 changed the title WIP: #669 - feat(services) - add service logs #669 - feat(services) - add service logs Jul 5, 2017
@WTFKr0
Copy link
Contributor Author

WTFKr0 commented Jul 5, 2017

Ok for me
I've tested that Logs are not displayed on dockerd 17.05
Feel free to test
I remove the WIP

deviantony
deviantony requested changes Jul 8, 2017
View reviewed changes
Copy link
Member

@deviantony deviantony left a comment

Choose a reason for hiding this comment

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

Small display issue, can you update it? See my comment.

app/components/service/service.html Outdated
@@ -72,6 +72,13 @@
<input type="text" class="form-control" ng-model="service.Image" ng-change="updateServiceAttribute(service, 'Image')" ng-disabled="isUpdating" />
</td>
</tr>
<tr>
Copy link
Member

Choose a reason for hiding this comment

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

The condition should be located on the <tr> element so that we don't display an empty row.

See the screenshots below:

portainer 12

portainer 13

deviantony
deviantony approved these changes Jul 8, 2017
View reviewed changes
Copy link
Member

@deviantony deviantony left a comment

Choose a reason for hiding this comment

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

LGTM

@deviantony deviantony added contrib/ready-to-merge and removed changes-required Waiting for an update of the contributor labels Jul 8, 2017
@deviantony deviantony merged commit 6df5eb3 into portainer:develop Jul 8, 2017
@deviantony
Copy link
Member

Thanks @WTFKr0 👍

@WTFKr0 WTFKr0 deleted the feat669-service-logs branch July 8, 2017 22:53
SvenDowideit pushed a commit that referenced this pull request Sep 17, 2021
@chiptus
fix(endpoints): add more wiggle room for checkin interval (#671)
90f0e27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deviantony deviantony deviantony 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
@WTFKr0 @deviantony