ASKBOT / askbot-devel

Askbot is a Django/Python Q&A forum. **Contributors README**: https://github.com/ASKBOT/askbot-devel#how-to-contribute. Commercial hosting of Askbot and support are available at https://askbot.com
Other
1.56k stars 627 forks source link

New installer 2019 #849

Closed martin-bts closed 3 years ago

martin-bts commented 4 years ago

Ok, for real now. :-)

I combined the work from https://github.com/ASKBOT/askbot-devel/pull/833 and https://github.com/ASKBOT/askbot-devel/pull/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 deployables module has all the things we want/can do with the installer. In deployables.objects one can see that we can:

In deployables.components one can see which files we want to deploy how. For instance, settings.py shall be a rendered with Jinja2.

The parameters modules has all the input/control related information. In parameters/__init__.py (and only there!) the parameter names are associated with validation classes. For instance, the parameter database_engine is validated by an instance of class DbEngine.

For each parameter we want validation there must (!) be a validation class defined. I structured the classes into the files cache.py, filesystem.pyand database.py for clarity, but this structure does not carry any semantics. ConfigManagers are used to create relationships between different parameters. This has been moved to parameters/configmanagers.py and as we can see, there aren't many relations i.e. there is not much work the ConfigManagers (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.

evgenyfadeev commented 4 years ago

Awesome! @martin-bts - I'm currently looking at merging this pull request, please don't make further pr's in this area meanwhile.

martin-bts commented 4 years ago

Oh, why didn't you tell me there are severe issues with this? a) I missed a patch or two when creating this PR b) There is undeterministic behaviour which makes the installer, and especially its tests fail, if your are unlucky. It appears I am assuming a guaranteed ordering of elements where I mustn't assume a guaranteed order.

evgenyfadeev commented 4 years ago

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:

1) remove all uses of import * - this method of import makes it hard to understand what is imported from where

2) fix failing test cases

3) pylint all files that you've created (PEP8)

4) restore interactivity of 'askbot-setup'. It used to work without raising exceptions by just tying "askbot-setup". Any required missing parameters would be solicited to be entered by hand. Currently it works only if one provides half a dozen parameters on the command line (tried with postgres) - this is much less usable than it was before. The old installer (give it a try perhaps) would ask for missing parameters one by one. If parameters are incorrect, the installer should be giving human readable feedback and not printing stack traces.

evgenyfadeev commented 4 years ago

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.

martin-bts commented 4 years ago

I am having trouble finishing this. The introduction of PostgreSQL 12 was maybe a bit too much.

  1. I replaced the from ... import * statements.

  2. I was able to reproduce two issues:

    • ExportUserDataTests expects testproject/testproject/askbot/upfiles/ exists before it is actually created.
    • The new approach at #845 is incomplete and loads the old incompatible script in askbot/management/commands/init_postgresql_full_text_search.py which is loaded from askbot/tests/test_page_load.py. Can you please name all the errors you got so that we can be sure that we got them all.
  3. tox -e lint does not yield any findings. Using pylint3 askbot/deployment/ gets a rating of 6.26/10 while askbot/ gets a rating a little under 3.00/10. Can you be more specific about which aspects of PEP 8 are particularly important to Askbot? Maybe we could add a config file for a linter to the project as well?

  4. Haven't worked on this yet. There shouldn't be any stack traces. Looks like broken code.

martin-bts commented 4 years ago

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.

evgenyfadeev commented 4 years ago

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:

1) Could you please fix askbot.tests.test_installer.FilesystemTests.test_project_dir ?

2) When asking for the DB host, port and password - hitting enter should set empty string value. this should also be clear from the comment printed just above the keyboard input prompt.

3) The same as 2) but for the caches settings.

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. PEP8 work can be postponed after the merge with the master branch.

The postgres full text search issues are now fixed.

martin-bts commented 4 years ago

I don't know why you think askbot.tests.test_installer.FilesystemTests.test_project_dir is broken. It has been working fine for me. However, I think this test used to work on Linux only, so I changed it to probably work on other OSs as well. Does this solve your issue?

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

martin-bts commented 4 years ago

@evgenyfadeev bump

evgenyfadeev commented 4 years ago

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 notifications@github.com wrote:

@evgenyfadeev https://github.com/evgenyfadeev bump

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ASKBOT/askbot-devel/pull/849#issuecomment-602504334, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAY5ARTEXHPVJUYLHEJHSTRI4ZN5ANCNFSM4I7SIEPA .

-- Askbot Valparaiso, Chile skype: evgeny-fadeev

evgenyfadeev commented 4 years ago

@martin-bts fyi I'm working on finishing this up in branch pr-849-wip.

martin-bts commented 4 years ago

@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?

evgenyfadeev commented 4 years ago

@martin-bts no, it's just to let you know. My branch contains yours.

martin-bts commented 4 years ago

No, it doesn't. Only everything up until (and including) January. My work from February, following your feedback, has not yet been merged.

teury commented 4 years ago

What version of Django do you use in this branch, they have not thought to upload to Django 3

evgenyfadeev commented 3 years ago

@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!

evgenyfadeev commented 3 years ago

@teury master branch now works with Python 3, Django 2 (no Django 3 yet, but would love to support).