New installer 2019 #849
New installer 2019 #849
Conversation
Otherwise it will always overwrite the DB config in settings.py
* The current idea is to have a dedicated class for each install parameter * A class implements acceptable() and ask_user() * ConfigManagers check acceptable() for each parameter and call ask_user() if/while acceptable returns False * On accepting an install parameter ConfigManagers can take actions in _remember(). This is used to (slightly) alter the behaviour of the classes belonging this ConfigManager
* Added askbot/tests/test_installer.py which currently only tests the askbot.deployment.parameters module * The 10 added tests throw various sets of install parameters at the parameters module w.r.t the database and cache settings. * The parameters module currently only checks the database_engine and cache_engine option for sensible values. For all other parameters, it only ensures that the user did provide parameters at all, not if they are sensible or not.
* Even though the default values are of type int, the parser arguments for cache_engine and database_engine are retrieved as strings. Added casts to int to fix this issue * First integration efforts indicated it makes sense to collect all ConfigManagers to make everything accessible via a single object the installer can use
* fixed import statement for SettingsTemplate * refactored an open() close() instance with "with open(..."
* The arguments added through _add_settings_args and _add_setup_args
were not sorted properly. They now are
* When running askbot-setup -h, the output depends on the ordering of
the code. To make the installer more usable I pushed the db and cache
related arguments to the end of the list and put the '--' names
before the '-' names.
* Three new arguments to askbot-setup_
--dry-run:
Print options_dict and exit jsut before the deployment would start
--use_defaults:
No function yet. There is supposed to be an explicit dict for an
Askbot default installation. This dict will be forced into options,
if this argument is provided
--no-input
No function yet. The idea is to make the installer fail instead of
asking the user for missing input, if this argument is provided.
* no integration yet
* lots of changes to existing code * also manages app_name now
* added more flexibility w.r.t. verbosity and interactive settings to ConfigManagers * ConfigManagerCollection is now derived from ConfigManager * AskbotSetup loads ConfigManagers during __init__
* correctly propagate verbosity settings from the installer instance into the managers and fields * propagate force settings from manager to fields * always run complete() on options (used to be omitted if --force)
* also did some beautifying on the installer itself
* both files will be removed before this work is finished * these are my current thoughts and first implementation * this is not integrated into Askbot. You can read deployments.md locally and run dev_django.py in a virtualenv to play around with the classes.
! Apparently the installer (and tox) requre the env var SECRET_KEY to be set in current 0.11.x upstream * created deploy_askbot_new() and use it as a drop in replacement for deploy_askbot() * modified deployed directory structure to include Evgeny's suggestion from ASKBOT#833 * Added new deployable EmptyFile to accomodate the empty log file
…ctory layout * see ASKBOT#833
* does not need to set DJANGO_SETTINGS anymore, because the installer does a better job deployoing the files now
* moved all generic logic and base classes into a base module
* removed all classes and logic from the deployables and parameters modules that are provided through the base module * import from base module
* added comments to main installer class to describe what the important statements do * ALL the relations between input parameters (their names) and validation classes are defined in parameters/__init__.py; no more looking in different places. Is there validation for a specific input? This file holds the anser! Do you want to add validation for another paramter? Do it there!
|
Awesome! @martin-bts - I'm currently looking at merging this pull request, please don't make further pr's in this area meanwhile. |
|
Oh, why didn't you tell me there are severe issues with this? |
* askbot/patches/__init__.py relied on some ancient method for obtaining the currently installed Django version => updated * Missing change: use the exported instances from askbot.deployment.parameters instead of creating new instances all the time * Missing change: add reset() to ConfigManger because for testing purposes we now need to reset ConfigManagers as we are not creating new instances for each test * Bugfix: ConfigManager._order(unsorted_list) would not return the result of its ordering, but the original unsorted_list, making this method effectively a noop. It now returns an ordered list as intended.
martin-bts
force-pushed
the
new_installer_2019
branch
from
6530ab9 to
5cbf4e7
Compare
|
Hi Martin, Happy New Year! I've made a serious effort to merge this commit, but encountered a number of issues and could not complete. However there is an intermediate result in branch pr-849-wip it contains your branch "new installer 2019", the tip of 0.11.x and some fixes that I've added to make the migrations run and the installer to succeed a basic installation with postgres (I have version postgres 12). If you would like this PR to be merged, could you please use branch pr-849-wip and address issues that I will list below. Issues:
|
|
Hey @martin-bts are you working on this? If you cannot finish this I will, just would not like to duplicate the effort. Cheers! This PR is the biggest showstopper on the way of releasing the Python3 version. Another one is having django-livesettings3 on pypi - I did ask the author to make a release, hopefully he does soon. |
|
I am having trouble finishing this. The introduction of PostgreSQL 12 was maybe a bit too much.
|
* as per ASKBOT#849 (comment) * untested so far !
* as per ASKBOT#849 (comment) * untested so far !
* as per ASKBOT#849 (comment) * untested so far !
|
I put some effort into the cli. Should be better now. You can pull the changes from the pr-849-wip branch on martin-bts/askbot-devel. |
|
Great, I've made an iteration on top of your work: https://github.com/ASKBOT/askbot-devel/tree/pr-849-wip There are now two items that would be awesome if you could address:
Everything else seems good, thanks a lot for your effort! I believe that with the three items taken care of we can merge this into the master branch. Below are just comments... Re: pylint - I've been running pylint on all new code, older code has a lot of pylint issues. The postgres full text search issues are now fixed. |
|
I don't know why you think I think the installer does now behave as you described it. However, in contrast to databases, where an empty value falls back to localhost, I think not providing a location for Memcached and Redis will simply result in a broken configuration. You can find the changes in my fork: https://github.com/martin-bts/askbot-devel/tree/pr-849-wip |
|
@evgenyfadeev bump |
|
Hi Martin, I'm a bit behind now, but will work on this over the weekend.
…On Mon, Mar 23, 2020 at 7:18 AM Martin ***@***.***> wrote:
@evgenyfadeev <https://github.com/evgenyfadeev> bump
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#849 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAY5ARTEXHPVJUYLHEJHSTRI4ZN5ANCNFSM4I7SIEPA>
.
--
Askbot
Valparaiso, Chile
skype: evgeny-fadeev
|
|
@martin-bts fyi I'm working on finishing this up in branch pr-849-wip. |
|
@evgenyfadeev Great! I cannot contribute to the Askbot repo directly, which is why I published my changes in a similarly named branch in my fork (https://github.com/martin-bts/askbot-devel/tree/pr-849-wip). Would you merge my pr-849-wip branch into your pr-849-wip branch? Or should I create a new merge request for that? |
|
@martin-bts no, it's just to let you know. My branch contains yours. |
|
No, it doesn't. Only everything up until (and including) January. My work from February, following your feedback, has not yet been merged. |
|
What version of Django do you use in this branch, they have not thought to upload to Django 3 |
|
@martin-bts , the installer now works. I've taken some ideas from this PR, but mostly rewritten. Closing as the issue is now resolved (and published on pypi). Thanks a lot! |
|
@teury master branch now works with Python 3, Django 2 (no Django 3 yet, but would love to support). |
Ok, for real now. :-)
I combined the work from #833 and #827 into a new branch and refactored it into what I think should become the new installer.
With the refactoring I wanted to separate logic and generics from the concrete things we do when we install Askbot.
The
deployablesmodule has all the things we want/can do with the installer. Indeployables.objectsone can see that we can:In
deployables.componentsone can see which files we want to deploy how. For instance,settings.pyshall be a rendered with Jinja2.The
parametersmodules has all the input/control related information. Inparameters/__init__.py(and only there!) the parameter names are associated with validation classes. For instance, the parameterdatabase_engineis validated by an instance of classDbEngine.For each parameter we want validation there must (!) be a validation class defined. I structured the classes into the files
cache.py,filesystem.pyanddatabase.pyfor clarity, but this structure does not carry any semantics.ConfigManagers are used to create relationships between different parameters. This has been moved toparameters/configmanagers.pyand as we can see, there aren't many relations i.e. there is not much work theConfigManagers (have to) do.The state of this is "works on my machine" and container installations do not yet work, as the container scripts expect a different file layout than the installer creates. This doesn't create problems for the default use case.