-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add support for templatePatch #499
Add support for templatePatch #499
Conversation
Hint: Since you changed the schema for |
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@the-technat ran make generate, thx! |
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@the-technat i pushed a commit that should have fixed the tests |
@KyriosGN0 it looks like the tests fail for 2.9. I'm pretty sure that's because templatePatch support was added in 2.10. Glancing at the code it looks though as if you have added logic to not use this feature if the server is 2.9. Could you reverify this? In my opinion since it's a feature supported in the last three minor versions we shouldn't have to spend too much time trying to get the tests green on 2.9 which is unsupported anyways. Instead we might want to wait for #472 which will remove 2.9 from the CI anyways. This just as a hint to make it easier for you 😄 |
@the-technat yeah i tried and it failed, will take a look in a couple of days and will test locally, thx! |
Signed-off-by: AvivGuiser <avivguiser@gmail.com>
@the-technat should work now |
I see, thanks @KyriosGN0 for your work! Will do some manual tests this weekend as you suggested and give it a final review... |
hey @the-technat did you have sometime to review my PR ? (did some basic testing myself and it worked) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry for the late reply and thanks for the bump @KyriosGN0. I just reviewed the PR and approved it + rebased the branch. If the tests pass it will be automatically merged soon. Thanks for your contribution! |
closes #368
still needs some manual testing