Skip to content

Conversation

@psaniko
Copy link
Contributor

@psaniko psaniko commented Aug 30, 2016

We need to toggle sticky behavior on/off based on different conditions. Would you be willing to integrate this change and if so could you check if the PR needs any more adjustments?

@harm-less
Copy link
Owner

harm-less commented Aug 30, 2016

Oh, nice feature ;)

From the looks of it the changes seem good, though I'm not sure if they work in every situation. I'm not sure if it also works if you toggle the enable back to true for example. But like I said, that's only from looking at the code instead of actually testing it myself.
That having said, can you make a couple of unit tests that accompany your feature? Let me know if you need any help with that.

@harm-less
Copy link
Owner

And let's not forget the documentation as well. But I can do that for you if you like.

@psaniko
Copy link
Contributor Author

psaniko commented Aug 31, 2016

Wanted to see if you like the impementation first :)

Added tests and documentation. Sorry for the commit confusion, I switched between work and personal.

@harm-less
Copy link
Owner

harm-less commented Aug 31, 2016

Good call ;)

Thanks for the additions, it's starting to take shape. The unit tests probably cover all your code, but not on the services necessarily. The "hlSticky" directive I think is covered very nicely, but the following service is not yet covered:

And btw, now that I think about it. What happens if you have a stack of 3 and the second one is turned off?

@psaniko
Copy link
Contributor Author

psaniko commented Sep 1, 2016

Thanks for pointing that out, I missed that. Should now be fixed and I also added some more tests.

@harm-less
Copy link
Owner

Looks good now from what I can see from the code! Hopefully within a few hours I can review it in more detail. Otherwise it will be tonight.

But once again, thanks for the feature ;)

@harm-less harm-less merged commit 8dfe72e into harm-less:master Sep 2, 2016
@harm-less
Copy link
Owner

All right, everything is done! Great feature mate ;)

It has been release in version 0.3.0, cheers.

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.

2 participants