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

Support for hot upgrades #305

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

guidotripaldi
Copy link

@guidotripaldi guidotripaldi commented Jul 23, 2019

Description

Adds hot upgrade support and documentation. Closes #195.

Licensing/Copyright

By submitting this PR, you agree to the following statement, please read before submission!

I certify that I own, and have sufficient rights to contribute, all source code and
related material intended to be compiled or integrated with the source code for Bootleg
(the "Contribution"). My Contribution is licensed under the MIT License.

NOTE: If you submit a PR and remove the statement above, your PR will be rejected. For your PR to be
considered, it must contain your agreement to license under the MIT license.

@rjanja
Copy link
Contributor

rjanja commented Jul 23, 2019

Hi @guidotripaldi, thanks for the PR! This looks great, and the documentation is fantastic!

This branch is now behind master and needs to be rebased in order to benefit from some testing pipeline improvements we've recently made. I'm happy to do so but this would require force-pushing to your branch, something you might not be expecting and that may cause a conflict for you.

After rebasing, the two missing pieces I see are

  • Documentation (not the moduledocs, which look great, but a separate docs section on hot upgrades)
  • Functional tests

I'm happy to work on either or both of the above and help get this across the line. Thanks again!

@rjanja rjanja changed the title Hot upgrade Support for hot upgrades Jul 23, 2019
@guidotripaldi
Copy link
Author

Hello @rjanja , thank you!
I have rebased the hot_upgrade branch and forced a push to origin, now there should be no more conflicts.
I've added also the mkdocs documentation, please let me know if it fits well, or if you prefer a different style/structure.

@rjanja
Copy link
Contributor

rjanja commented Jul 26, 2019

Looks great @guidotripaldi ! Thanks for all your work on this. I'll spend some time with it over the weekend.

@rjanja
Copy link
Contributor

rjanja commented Aug 6, 2019

I apologize it's taking me longer to review this than I expected. I'll be pushing up some documentation changes soon but still need to work out the functional tests. On the bright side, I've been able to use this to do some hot upgrades of a really simple app :)

@guidotripaldi
Copy link
Author

no problem, take your time

@rjanja rjanja self-requested a review August 11, 2019 00:26
@rjanja rjanja self-assigned this Aug 11, 2019
@rjanja rjanja added this to the 0.13.0 milestone Aug 11, 2019
@lessless
Copy link

lessless commented Oct 12, 2019

Hey guys,

Just don't want review of this amazing feature to fold over the Christmas ;)

@lessless
Copy link

Hi @rjanja, @guidotripaldi

Would you please add a bit more gas to get this through review process?

Thanks.

@guidotripaldi
Copy link
Author

Hello @rjanja, is there something I can do to help you in the review process for this long pending PR?

Hi @rjanja, @guidotripaldi

Would you please add a bit more gas to get this through review process?

Thanks.

@rjanja rjanja removed their assignment Feb 18, 2020
@holetse
Copy link
Member

holetse commented Feb 20, 2020

@guidotripaldi Thanks for taking the time to pull this together, and for sticking with it for all these months. We'd like to incorporate these changes but there are still two outstanding issues:

  • Conflicts with the master branch the must be resolved (please use a rebase) prior to merging.
  • The new functionality needs unit and integration tests.

The tests are the bigger hurdle, as the conflicts look trivial. Unfortunately we do not have the time right now to add the missing test coverage in-house, but we do have the time to support someone else adding it. Examples on how to test tasks can be found here (both unit and functional tests): https://github.com/labzero/bootleg/tree/master/test/bootleg/tasks. Our goal is 100% coverage for these sorts of changes, as reported by coveralls.

@lessless
Copy link

hey @guidotripaldi - let me know if you would like to pair on that

@guidotripaldi
Copy link
Author

hey @guidotripaldi - let me know if you would like to pair on that

Hello lessless, Thank you for being available. Unfortunately I have no time at the moment to learn how to develop the required unit and integration tests, so it would be wonderful if you can take over this job

@lessless
Copy link

Same thing here. I would love to use hot upgrades in bootleg but don't have time to figure out what exactly should be done here.

@holetse I guess your situation didn't change as well, isn't it?

@lessless
Copy link

@holetse @rjanja can you guys please help to define expectations and I'll try to find some time to get this merged in.

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.

Hot upgrade support
4 participants