Update project structure

[cormier]

Update the project structure to add the following:

• mattermostgithub for the project directory
• docs for everything related to the documentation.

I believe that this will help begin work on #4. We will be able to have all files that need to be tested in mattermostgithub and all tests in a tests directory.

Dependencies

This PR depends on #33. I will update this PR once #33 is merged.

[cormier]

Okay I have rebased this PR on top of master, so it is ready for review!

[ptersilie]

Looks good. I'm just wondering how we can make sure that this update doesn't break other people's setup (for instance if they have a cronjob that does git pull without looking at the changes), or if we are reaching a point now where keeping everything compatible is not worth the effort and we can expect people to test after they pull.

[cormier]

What are the different possible setups? I see Docker and local install.

For Docker, I think that breaking things is not much of an issue as long as we update the Dockerfile properly.

For local install, this PR will break existing setups because it changes how you start the application. If the person looks at the changes, I think it will be easy to figure out, but if it's git pull in a cronjob it will just break things.

In my opinion breaking things will be unavoidable but there are ways we can manage this. For example we could:

1. Start making and tagging releases: for breaking changes we could just bump the major versions and people would normally expect major changes.
2. Create a python package out of this project and publish it on PyPI. People would be able to just pip install mattermost-github-integration and run the provided executable. The executable would be the main user interface, so structure changes like this wouldn't break setups.

[cormier]

I added a commit that packages the project with distutils to show what I mean. The idea is to let people run the project with the following sequence:

virtualenv-3 venv
. venv/bin/activate
pip install mattermost-github
flask run --config=path_to_config

The config part has not been implemented yet, so it still expects a config.py to be present in the project root. Let me know what you think of this approach. If you think we should go forward with PyPI packaging I'll finish the work.

[ptersilie]

A PyPI project surely can't hurt. But I also don't want to break peoples setup just yet. So on top of the pypi project we could create a server.py in the main folder, which basically imports the mattermostgithub/server.py (which we should rename at that point), and runs the server. So the whole thing can still be run using python server.py for now. At some point, when the pypi project has been established we can maybe make more breaking changes.

I also wanted to suggest renaming mattermostgithub to lib. Maybe also add a folder bin/ which contains something like mgi.sh which does python2 server.py so it's even easier in the future to change things without breaking peoples setup. To be honest, I should have just provided a bin/run.sh file from the start. But I can only learn from my mistakes.

[cormier]

I think we should keep mattermostgithub as the name of the package. The Packaging and Distributing Guide suggests a single package that has the same name of the project or a close name. This directory will be installed in site-packages and I think lib is too generic for that.

Root level server.py is a good idea to support those who want to keep running the project straight from the git repo. It would also be transparent for python2 users: they will be able to just run the top level server.py with their python2 interpreter (and retain compatibility). For that reason I'm not convinced by the need of a mgi.sh but maybe I'm missing something. :smile:

[ptersilie]

The package naming makes sense. I didn't really know how pypi packaging worked. But given what you said I agree that lib is not a good name. :D

I just thought a bin/mgi script would have been helpful from the start as then we could have renamed anything we'd like without breaking anyones setup. But now it's too late for that. I'm happy to just keep the server.py in the root for backwards compatibility and then do all the other changes that you proposed on top. :)

[cormier]

Yea that's true it would have definitely been helpful to have such an entry point. Okay so to finish this I will:

• [ ] Add a server.py file at the root of the project
• [ ] Add support for a config argument to let users configure the config file at runtime

[ptersilie]

Thanks a lot for spending so much time on this. This looks good. Just one thing, now that we invested so much into keeping this backwards compatible, we shouldn't break it again by forcing people to use environment variables.

I guess we'd need a fallback solution, e.g.

if os.environ.get('MGI_CONFIG_FILE'):
    # ...
else:
    config = imp.load_source('config.py', "rootfolder")

[ptersilie]

Do we really need environment variables in the first place. I'd like to keep configuration down to a minimum (i.e. adjusting the config.py). Now one has to export environment variables and also remember to put this into the bashrc or the script that starts this service (I'm using systemd to autostart this plugin) or otherwise the variables will be gone after a reboot.

[cormier]

Thanks for reviewing this.

This is fully backward compatible. Using environment variables is optional: if MGI_CONFIG_FILE is specified, the package will load the specified file and expose it in the scope as mattermostgithub.config, so server.py and payload.py will just import it as usual. If MGI_CONFIG_FILE is not set, server.py and payload.py will just import the config.py in mattermostgithub. Users who just want to do python server.py from the root directory can continue to do so, and the project will work as usual.

I'm not too fond of configuring via environment variables, but I implemented it that way to be consistent with the way Flask looks for the application to run when it is invoked by flask run. Basically I would like users to be able to do the following:

pip install mattermost-github
vim config.py
... edit the config file ...
export FLASK_APP=mattermostgithub
export MGI_CONFIG_FILE=config.py
flask run

Other users would still be able to clone the repo, create a config.py from the config.template and start the app with python server.py.

[jaheba]

Just to add my two cents: I think it's generally a good idea to specify which config file to use, as @cormier suggested. If it doesn't break anything for existing users it sounds like a good solution to me.

Do you want to add a mgi script, because I haven't found anything like it in the setup.py.

[cormier]

Thanks for reviewing this PR.

I made the requested changes in a single commit that we can squash if we decide to merge. We still need to figure out what to do with the collaborator listings.

Also, I did not include a mgi script because flask already provides a flask run that we can parametrize with environment variables.

[ptersilie]

I think it would be nice to list every contributor in the package, but I don't know what other projects do.

As mentioned above the current version is not fully compatible with old setups as config.py is located in the root folder. So we probably want to do something like

try:
    import config
except ImportError:
    from mattermostgithub import config

(or the other way around)

[cormier]

As for authors:

• For flask it's Armin Ronacher
• For Django it's the Django Software Foundation

I think Software Development Team is alright: it is also consistent with LICENSE.txt.

[ptersilie]

We can also just say Mattermost Github Integreation Developers, which is what some projects do.

[cormier]

Okay let's do this. I do not have a strong opinion on this issue :)

[cormier]

Is info@soft-dev.org a valid email?

[ptersilie]

Should be, but I don't think we want to use it for this. I think we can just leave the email field empty.

[jaheba]

Flask also has this file

Maybe we just do something similar and do Mattermost Github Integreation Developers in the setup.py

[ptersilie]

Okay, let's just do Mattermost-Github-Integration Developers and if wanted we can also add an AUTHORS document, although that would have to be maintained as well from then on.

[jaheba]

Looks good to me so far, I leave you two to it :)

[ptersilie]

I'm still getting import errors for config, as the previous setup had the config.py file in the root directory. Apart from that I'm happy with this PR.

[cormier]

One should now be able to specify the configuration by either:

• Placing a config.py file in the root directory
• Specifying a config file with the MGI_CONFIG_FILE environment variable

[cormier]

Okay we can do this. :smile:

[cormier]

Hi @ptersilie, I was wondering if there was anything left to do before merging this PR. Starting the server using a config.py placed in the root directory should now work with my latest commit.

[ptersilie]

Hey @cormier, I think this looks good now. Just two more things:

1) I think the Dockerfile needs to be reverted back, as the command python mattermostgithub/server.py doesn't work. 2) How do we get the package uploaded to pypi, and how do we maintain it there?

[cormier]

You are right, the Dockerfile has to be fixed.

For PyPI: we need to package the project and upload it as per the Packaging and Distributing guide. If you wish, this is something I can do: you can create the account for the project and share the password with me (you can find my GPG key is here)

[ptersilie]

There more I think about it, there more this pypi setup looks like unnecessary work with little benefit. I've been looking at https://github.com/NotSqrt/mattermost-integration-gitlab, which is the gitlab plugin initially developed by the mattermost team (and which I based this plugin on). Their installation guide installs the plugin using pip install git+https://github.com/NotSqrt/mattermost-integration-gitlab. Why don't we just do the same? This way we don't have to manage a pypi account and don't have to keep updating pypi with new releases. What do you think?

[cormier]

If we do this we won't be able to specify a python package version in requirements but a git commit, a git tag or a git branch. We could still tag versions in git, have them match the version defined in setup.py, and ask people to clone a specific tag to obtain a specific version.

E.g.

pip install git+https://github.com/NotSqrt/mattermost-integration-gitlab would retrieve the dev version (master)

pip install git+https://github.com/NotSqrt/mattermost-integration-gitlab@v1.0 would retrieve the tag "v1.0" which could be a production version.

We can do this, since this project is standalone and I doubt many people will want to have their python project depend on it. We can upload to PyPI later if someone files an issue and explicitely asks for it?

[ptersilie]

I think I'm fine with that. I am not planning to break master, so I think pulling the dev version is what most users want to do. Maintaining releases and making decisions about what goes into the next release and what not is not something I feel (for now) is worth doing on a small project like this. But I agree, if this is something that makes sense doing in the future we can always introduce releases then and then also start adding packages to pypi.

[cormier]

Okay! I believe that in this case, all we need to do is update the readme.

[ptersilie]

Updated. If you are happy, I will merge! :)

[cormier]

Superb!

[ptersilie]

Oh, just noticed, if users installed via pip, how do they run the thing? We should state that in the Readme as well. ;)

[ptersilie]

And also, how to they create a config.py and where do they store it?

[cormier]

Either by running server.py in the package directory (should be ./env/lib/python3.x/site-packages/mattermost-github/server.py if installed in a virtualenv called env) or with the flask command:

export FLASK_APP=mattermostgithub
export MGI_CONFIG_FILE=/path/to/config.py
flask run

[cormier]

config.py should be created from the template and placed in the directory of the user's choice

[ptersilie]

That's a lot less straight forward than I hoped it would be. We will have to point them to where they can copy the config.template from. But I think the pip installation folder varies depending on the Linux distribution (and possibly also depending on if they installed with --user or not).

I don't think telling them to run python ./env/lib/python3.x/site-packages/... is an acceptable solution. Noone would want to do that, especially if just using github is so much quicker.

[cormier]

Exactly. I think we should suggest two ways:

1. git clone and use config.template from the repo
2. pip install and copy config.template from the repo (or copy/paste the example) and then use flask run.

[ptersilie]

I guess I can live with that. I updated the README accordingly.

There's just two more things that are unclear to me: 1) I don't have a flask command on any of my machines, even though flask is installed on all of them. So there seems to be some extra steps involved to run it that way. But maybe that's just my machines. 2) I assume FLASK_APP needs to point to the folder containing the plugin. So export FLASK_APP=mattermostgithub unless we are still in the folder. Or does the installation through pip take care of that?

[cormier]

I think it's a matter of setup. Did you use virtualenv or did you do a global setup? If you use virtualenv, the flask command will be available on the path when the environment is active, and mattermostgithub will be available on the python path (if installed with pip)

[cormier]

You should be able to just do:

virtualenv-3.5 env
. env/bin/activate
pip install git+git://github.com/erudit/mattermost-github-integration@update-project-structure
export FLASK_APP=mattermostgithub
export MGI_CONFIG_FILE=/path/to/config.py
flask run

[ptersilie]

Ah, no I did not. That must be the problem then. Would you mind updating the README with those instructions? Then we can finally merge this. :)

[cormier]

Okay, here's a commit that updates the readme!

[ptersilie]

Finally merged! :) Thanks again for your contribution! :+1:

[cormier]

Thanks :smile:

I will now rebase #37 on top of this