WIP | Dev: Initial bit integration #1029

mtreacy002 · 2021-03-03T06:42:20Z

Description

Add initial integration for BIT backend to MS backend

Fixes #1028

Type of Change:

Code
Quality Assurance

Code/Quality Assurance Only

This change requires a documentation update (software upgrade on readme file)
New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Complete the local development setup as instructed on BIT wiki page. For ms-backend-server bit version, run it from this PR branch.
run BIT backend server FIRST to create the schemas inside postgresql. then run MS backend server from this PR branch.
Register a user and see success message.

4. run tests by executing the command `python -m unittest discover tests` and see all test run successfully

Login on BIT Swagger UI and make call requests related to MS users service (on gif below GET user/personal_details and GET users/{user_id} were made as examples)

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
Any dependent changes have been merged
Update requirements.txt

Code/Quality Assurance Only

My changes generate no new warnings
New and existing unit tests pass locally with my changes

Additional note
For the giithub action to run test successfully, the main.yml need to be modified to include adding postgresql plugin and creating test database with the 2 schemas as it was done on Travis.yml on BIT backend repository.

app/database/models/mentorship_relation.py

config.py

epicadk · 2021-03-03T08:59:36Z

config.py

+        db_user_arg=os.getenv("DB_USERNAME"),
+        db_password_arg=os.getenv("DB_PASSWORD"),
+        db_endpoint_arg=os.getenv("DB_ENDPOINT"),
+        db_name_arg=os.getenv("DB_NAME"),


Then why not use the DB_NAME variable instead of refetching them from the environment variables?

I'm not sure I'm following, 🤔.

This was a continuation from the above comment. On line 44 there is a variable called DB_NAME that has this value, then why not use that here instead of fetching the environment variable again.

Fixing it now. Thanks for pointing that out 😊

@epicadk , done. Can you please check again? Thanks

Looks good 💯

epicadk · 2021-03-03T09:03:15Z

run.py

@@ -16,6 +19,8 @@ def create_app(config_filename: str) -> Flask:

    migrate = Migrate(app, db)

+    cors.init_app(app, resources={r"*": {"origins": "http:localhost:5000"}})


Does BIT required access to all endpoints of the mentorship backend or can we give access to specific endpoints instead of all the endpoints?

Yes, @epicadk. With users functionaliities and mentorship relation are managed by Mentorship System, BIT does need to have access to all Mentoship System api endpoints 😉.

Do you think we should fetch the origins property based on the build type i.e. production, dev, local etc.?

@epicadk, I think it's best to keep it as it for now, coz majority of contributors work on local machine atm. When we have remote servers for dev and production, we can revisit this origin property.

mtreacy002 · 2021-03-03T13:46:35Z

@epicadk and @annabauza . I'm trying to set up postgresql inside github action but having trouble getting it right.

Here's the modification to main.yml i've done.
From the PR description above (Additional Content) you can see how I've done it using Travis CI
PS: I've never written CI/CD to github action before so, any help would be appreciated.

epicadk · 2021-03-03T16:56:04Z

It won't let you create a database for some reason it is setup just fine. Is there anyone to set these from the python code?

mtreacy002 · 2021-03-03T22:11:03Z

It won't let you create a database for some reason it is setup just fine. Is there anyone to set these from the python code?

can you explain what you mean by setting up on the python code, @epicadk ?

epicadk · 2021-03-04T04:05:47Z

It won't let you create a database for some reason it is setup just fine. Is there anyone to set these from the python code?

can you explain what you mean by setting up on the python code, @epicadk ?

Like we do for the mentorship system db.create_all() over here

mtreacy002 · 2021-03-04T07:11:30Z

It won't let you create a database for some reason it is setup just fine. Is there anyone to set these from the python code?

can you explain what you mean by setting up on the python code, @epicadk ?

Like we do for the mentorship system db.create_all() over here

I've done that, @epicadk , but still facing error 😓

mtreacy002 · 2021-03-04T13:05:22Z

Update @epicadk :
I'm now able to create test db and connect to Postgres in the build 🎉 💃
But now I'm stuck with install dependencies as shown below. Any idea why this happens? Does this have something to do with permission level to the Mentorship System GitHub repo? cc @isabelcosta

I also noticed that when the PR #654 for initial MS migration to GitHub action was merged, the Run test workflow did not get triggered. It was only triggered on the PR after (#655). I wonder why 🤔...

epicadk · 2021-03-13T13:35:15Z

@mtreacy002 could you avoid force pushing the changes? Kinda makes it harder to review. I had discussed this with @isabelcosta in the QA open session and I think the conclusion was that only while merging the maintainer needs to follow the commit guidelines given in the contributing guidelines. cc @isabelcosta . 😅

mtreacy002 · 2021-03-13T13:41:26Z

@mtreacy002 could you avoid force pushing the changes? Kinda makes it harder to review. I had discussed this with @isabelcosta in the QA open session and I think the conclusion was that only while merging the maintainer needs to follow the commit guidelines given in the contributing guidelines. cc @isabelcosta . 😅

Ok, it makes sense actually 😁. It's just a habit from squashing commits, I guess (which I actually forgot to do on the latest push 🤣). Will just do normal push the next one 😉

mtreacy002 · 2021-03-13T13:43:29Z

Oh, on the codecov report, I check the anitab codecov.io, and it looks like it's passing, so I'm not sure why it says failed here 🤔

epicadk · 2021-03-13T13:45:13Z

I don't have much knowledge of the subject 😅. I think @devkapilbansal can help out here.

mtreacy002 · 2021-03-13T13:47:15Z

I don't have much knowledge of the subject 😅. I think @devkapilbansal can help out here.

no worries..., @epicadk 😉. @devkapilbansal can you please share some input 🙏.

devkapilbansal

@mtreacy002 this PR consists of 5 commits but only one is relevant. All four are possibly because bit branch is not updated with develop branch.
So they should be removed.
Otherwise it is good for me.

replace db uri add Flask-Cors to requirements.txt linting with black formatter remove fetch env variables duplication change pull-push branch for github action to bit branch add postgresql test db add shell_context_processor Add -h postgres to psql command Place PGPASSWORD at the start of psql command Add env for running test * Update main.yml (Patch #21) Co-authored-by: Aditya Kurkure use non-macOS syntax for db_uri and add db envs Add postgres db url for github action Add test for db_test_uri remove hard coded test db uri Add psycopg2 Fix db type to postgres+psycopg2 add env to Generate coverage report move env to global runner focus pytest cov on tests folder Remove print command

mtreacy002 · 2021-03-13T21:50:44Z

@mtreacy002 this PR consists of 5 commits but only one is relevant. All four are possibly because bit branch is not updated with develop branch.
So they should be removed.
Otherwise it is good for me.

Done, @devkapilbansal , I removed the other 4 commits. It was initially to update bit branch with develop. But the maintainers can do that from their side too 😉

mtreacy002 · 2021-03-13T21:56:47Z

@epicadk , should I also integrate your push for docker change in here? Or that can be done after?

epicadk · 2021-03-14T04:33:03Z

@epicadk , should I also integrate your push for docker change in here? Or that can be done after?

It can be done after. I'll send open the pr on this repository.

mtreacy002 · 2021-03-14T05:41:09Z

@devkapilbansal and @epicadk , can you please re-approve this PR now so it can be merged? That way the contributors can push directly to upstream bit branch instead of to my fork ms repo. Thanks again 😉

epicadk

💯

isabelcosta

Looks good to me! @mtreacy002 🤗

isabelcosta · 2021-03-14T17:08:13Z

@mtreacy002 this is working for your specific case right? Should we test this again? or pass that, because you were already using this for BIT?

devkapilbansal

LGTM 👍

P.S.:- I haven't tested it

devkapilbansal · 2021-03-14T17:51:54Z

@epicadk, can you please tell me how to fix the failed codecov? Also, why is the codecov report is not showing for this PR just like it is shown on the other PR for develop branch (e.g. #1039)?

@mtreacy002 @epicadk looking into the codecov reports, it fails because of changes in run.py file. Also, in workflows, branches mentioned is develop. That's why codecov report is not shown here

mtreacy002 · 2021-03-15T01:28:47Z

@mtreacy002 this is working for your specific case right? Should we test this again? or pass that, because you were already using this for BIT?

@isabelcosta , I believe since the GitHub action shows that the build is passing here we won't need to do the manual testing again. I also have been using this branch for my BIT for MS backend server and able to use existing features as normal.
PS: I've added a gif on successful calls to MS users REST API from BIT Swagger UI using this PR branch under How has this been tested section.

@epicadk, can you please tell me how to fix the failed codecov? Also, why is the codecov report is not showing for this PR just like it is shown on the other PR for develop branch (e.g. #1039)?

@mtreacy002 @epicadk looking into the codecov reports, it fails because of changes in run.py file. Also, in workflows, branches mentioned is develop. That's why codecov report is not shown here

@devkapilbansal, can you tell me where the develop branches being mentioned in workflows that causing the codecov fails? as I've changed this to bit in the main.yml on this pr.

Are you talking on the codecov.io settings of AnitaB? Perhaps this must be done from the admins side since they have the authority on that?

mtreacy002 · 2021-03-18T01:59:18Z

@devkapilbansal , @epicadk , can you please let me know what else I need to do for this PR? If nothing else needed, we can label this issue Ready to merge so the maintainer can merge it to the MS bit branch.

epicadk · 2021-03-18T04:41:37Z

@devkapilbansal , @epicadk , can you please let me know what else I need to do for this PR? If nothing else needed, we can label this issue Ready to merge so the maintainer can merge it to the MS bit branch.

We have code approval . Now it needs testing . Busy with college this week i'll test it tonight.

epicadk · 2021-03-18T18:58:12Z

Okay I just realized I'll need to have postgreSQL installed to test this which I haven't installed yet.

mtreacy002 · 2021-04-11T11:41:31Z

Will close this issue as we're moving to full BIT-MS integration as discussed on BIT Open Session Sunday, 11th April.

mtreacy002 requested a review from isabelcosta March 3, 2021 06:42

mtreacy002 force-pushed the initial-bit-integration branch from 8988f98 to f66749b Compare March 3, 2021 07:02

mtreacy002 changed the title ~~Initial bit integration~~ Dev: Initial bit integration Mar 3, 2021

mtreacy002 force-pushed the initial-bit-integration branch from f66749b to fe7a5f3 Compare March 3, 2021 07:22

epicadk reviewed Mar 3, 2021

View reviewed changes

app/database/models/mentorship_relation.py Show resolved Hide resolved

epicadk reviewed Mar 3, 2021

View reviewed changes

mtreacy002 force-pushed the initial-bit-integration branch 6 times, most recently from 6bdac97 to c0c7e58 Compare March 3, 2021 13:38

mtreacy002 force-pushed the initial-bit-integration branch 3 times, most recently from 56a531f to ee9f17c Compare March 4, 2021 07:09

mtreacy002 force-pushed the initial-bit-integration branch 8 times, most recently from 9c868ff to 6ee8b47 Compare March 4, 2021 13:00

mtreacy002 force-pushed the initial-bit-integration branch from 6ee8b47 to 8a78c49 Compare March 4, 2021 13:07

mtreacy002 added the Help Wanted Extra attention is needed. label Mar 13, 2021

devkapilbansal reviewed Mar 13, 2021

View reviewed changes

mtreacy002 force-pushed the initial-bit-integration branch from 3ab6260 to 928bbe5 Compare March 13, 2021 21:34

mtreacy002 requested a review from devkapilbansal March 13, 2021 21:50

epicadk approved these changes Mar 14, 2021

View reviewed changes

isabelcosta approved these changes Mar 14, 2021

View reviewed changes

devkapilbansal approved these changes Mar 14, 2021

View reviewed changes

epicadk added Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. and removed Status: Needs Review PR needs an additional review or a maintainer's review. labels Mar 18, 2021

mtreacy002 mentioned this pull request Mar 24, 2021

Added postgre to database_uri #515

Closed

4 tasks

mtreacy002 added the Status: On Hold Issue or PR needs more info, a discussion, a review or approval from a Maintainer/Code Owner. label Apr 11, 2021

mtreacy002 changed the title ~~Dev: Initial bit integration~~ WIP | Dev: Initial bit integration Apr 11, 2021

mtreacy002 closed this Apr 11, 2021

		@@ -16,6 +19,8 @@ def create_app(config_filename: str) -> Flask:

		migrate = Migrate(app, db)

		cors.init_app(app, resources={r"*": {"origins": "http:localhost:5000"}})

WIP | Dev: Initial bit integration #1029

WIP | Dev: Initial bit integration #1029

Conversation

mtreacy002 commented Mar 3, 2021 • edited Loading

Description

Type of Change:

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 commented Mar 3, 2021 • edited Loading

epicadk commented Mar 3, 2021

mtreacy002 commented Mar 3, 2021

epicadk commented Mar 4, 2021 • edited Loading

mtreacy002 commented Mar 4, 2021

mtreacy002 commented Mar 4, 2021 • edited Loading

epicadk commented Mar 13, 2021

mtreacy002 commented Mar 13, 2021

mtreacy002 commented Mar 13, 2021 • edited Loading

epicadk commented Mar 13, 2021

mtreacy002 commented Mar 13, 2021 • edited Loading

devkapilbansal left a comment

Choose a reason for hiding this comment

mtreacy002 commented Mar 13, 2021

mtreacy002 commented Mar 13, 2021

epicadk commented Mar 14, 2021

mtreacy002 commented Mar 14, 2021

epicadk left a comment

Choose a reason for hiding this comment

isabelcosta left a comment

Choose a reason for hiding this comment

isabelcosta commented Mar 14, 2021

devkapilbansal left a comment

Choose a reason for hiding this comment

devkapilbansal commented Mar 14, 2021 • edited Loading

mtreacy002 commented Mar 15, 2021 • edited Loading

mtreacy002 commented Mar 18, 2021

epicadk commented Mar 18, 2021 • edited Loading

epicadk commented Mar 18, 2021

mtreacy002 commented Apr 11, 2021

mtreacy002 commented Mar 3, 2021 •

edited

Loading

mtreacy002 Mar 3, 2021 •

edited

Loading

mtreacy002 commented Mar 3, 2021 •

edited

Loading

epicadk commented Mar 4, 2021 •

edited

Loading

mtreacy002 commented Mar 4, 2021 •

edited

Loading

mtreacy002 commented Mar 13, 2021 •

edited

Loading

mtreacy002 commented Mar 13, 2021 •

edited

Loading

devkapilbansal commented Mar 14, 2021 •

edited

Loading

mtreacy002 commented Mar 15, 2021 •

edited

Loading

epicadk commented Mar 18, 2021 •

edited

Loading