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
Reorganize the folders to easier to deploy and bug fixes. #209
Conversation
This maybe need some modify for the unit tests. |
It seems like the codes use a lot class definition with super-with-arguments, should we change to the newer super() without arguments? If not maybe we can disable the R1725 pylint check? |
@bkueng Please review my code whether is correct, just wondering whether the modified pylint should be fixed. Maybe need to add nginx example ? |
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.
Thanks for looking into this, and fixing CI, much appreciated.
I'm ok with the directory move, but there's more things that need updating (README and script paths)
Currently the app depends on the config_default.ini/config_user.ini files to configuring the app, and however with docker/docker-compose it must need to use .env file, @bkueng as you said that maybe we can use a single configuration file for the app. What do you think about this? Another way to simplify this is to use only .env and for plot_app and etc, we can use python-dotenv instead of ini file. |
If we keep the 2 separate for now (and document it) that is ok. Later we can consider |
@bkueng Thank you so much, Then we can just document it, I will try to remove some redundant files. |
@bkueng bkueng I have tested on AWS server with a valid domain name for setup, and use cron to renew the SSL certs, I think it suppose to be no problem except for the current conflicts. Can you review my code?
|
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.
Looks pretty good, can you rebase and clean up the commits?
Added nginx basic authentication support.
Is that something you make use of?
6b52bb9
to
a576359
Compare
@bkueng I have rebased and clean up the current branch. Please have a look.
Yes i added for my use case. Of course we can remove it but I think there will be some body who don't want to show their flight reports, so I just add an example in the nginx. |
8da976c
to
60583ae
Compare
I still see conflicts though |
60583ae
to
e5c2374
Compare
@bkueng Sorry I thought rebase within the branch, I have pull the upstream/flight_review and rebase again. please have a look. I moved pylintrc to app for run_pylint.sh run properly. |
e5c2374
to
516e30e
Compare
hmm... let me fix the CI/CD |
cd0b4c0
to
a058ce4
Compare
@bkueng I have fixed the CI, please have a final check.^^ |
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.
Looks like the config file is not at the right place yet (move to app
as well, and update the readme). Can you test this w/o using docker as well?
Okay i will test with the python app w/o docker |
seems like it show the following errors.
|
It seems like that is the problem of pyulog version.
|
…ocker and added docker build gh-action test.
…following the pylint warning guide to fix CI/CD pylint test.
9e80e9a
to
f844069
Compare
@bkueng I have done the test w/o docker, The web application seems like working ok except the above warnings. Maybe this is cause by my ulg file. Please have a look again. |
Thanks, the warnings are expected because the log does not contain all data. |
…l.conf for challenging let's encrypt, added support nginx to use basic authentication, tested with AWS docker environment. Fix run_pylint.sh and move pylintrc to app. Fix CI/CD. Moved config_default to app for usage w/o docker.
f844069
to
4a8584e
Compare
@bkueng Thank you for checking and i fixed the config_default.ini's storage_path and also for docker-compose files. |
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.
Good to merge!
I'm trying to reorganize the folders to include the docker nginx for docker deployment.