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

Fixed upload artifacts to release and set iptables rules #388

Merged
merged 2 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@soffokl
Copy link
Member

commented Sep 19, 2018

No description provided.

@soffokl soffokl requested review from tadovas, Waldz, zolia and vkuznecovas Sep 19, 2018

@@ -34,17 +34,9 @@ func (service *serviceIPTables) Add(rule RuleForwarding) {
}

func (service *serviceIPTables) Start() error {
err := service.ipForward.Enable()
if err != nil {

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Maybe we should skipp this error here, in high level logic.
But lets have ipForward.Enable not swelling errors

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Author Member

Reverted error returning

@soffokl soffokl force-pushed the release-03-fixes branch from f4f0d93 to bd4d163 Sep 19, 2018

return err
}

service.ipForward.Enable()

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Warning logging whould be exactly here in high logic.
So that everybody understands why we swallow errors

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Author Member

We have logging in the Enable, do you want to duplicate it or move it to Start only?

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Author Member

Added extra log line.

@Waldz

Waldz approved these changes Sep 19, 2018

@@ -40,15 +40,11 @@ func (service *servicePFCtl) Add(rule RuleForwarding) {
func (service *servicePFCtl) Start() error {
err := service.ipForward.Enable()
if err != nil {
return err
log.Warn(natLogPrefix, "Failed to enable IP forwarding: ", err)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 19, 2018

Member

Now it's double logging, is not it?
Because service.ipForward.Enable() logs extra warning

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Author Member

yeah, it prints messages from both functions.

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 19, 2018

Author Member

I think we can leave it this way for the release.

@Waldz

Waldz approved these changes Sep 20, 2018

@zolia

zolia approved these changes Sep 20, 2018

@soffokl soffokl merged commit 5027e5c into release/0.3 Sep 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@soffokl soffokl deleted the release-03-fixes branch Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.