DalgoT4D / prefect-proxy

GNU Affero General Public License v3.0
0 stars 7 forks source link

Remove unused prefect-related functions #111

Closed kauxp closed 3 months ago

kauxp commented 3 months ago

Addressing https://github.com/DalgoT4D/DDP_backend/issues/542

Removed get_airbyte_connection_by_blockname and get_airbyte_connection_block_id function and their related tests.

fatchat commented 3 months ago

@kauxp yes exactly like this. did the (remaining) tests pass as well?

Ishankoradia commented 3 months ago

@kauxp this looks good, test cases are passing !! Thank you for contributing.

Could you go ahead remove the remaining unused code ? you can refer to the same sheet you have been looking at. Let me know if you face any issues.

PS - will fix the github action workflow by tonight.

kauxp commented 3 months ago

Did the (remaining) tests pass as well?

Yes, the rest of the test cases passed after the code changes, but I have verified that there are 2 cases which were failing before removing the unused functions, and those tests are not related to my changes. I am attaching those test cases below:

FAILED tests/test_service.py::test_create_dbt_core_block_success - proxy.exception.PrefectException: failed to create dbt cli profile
FAILED tests/test_service.py::test_create_dbt_core_block_exception - AssertionError: assert 'failed to cr...t cli profile' == 'failed to cr...core op block'
kauxp commented 3 months ago

Could you go ahead remove the remaining unused code ?

I think removing all the functions in one PR might be too extensive for review. Should I create separate PRs for each function, or should I create a single PR?

fatchat commented 3 months ago

no no we'll review it don't worry

the error you are getting can be fixed by running the tests like this

GOOGLE_APPLICATION_CREDENTIALS="service_account.json" pytest --asyncio-mode=auto

You will need a dummy service_account.json file... need to create a GCP account, create a service account, don't need to add any permissions, create a JSON key and download ... bit of a pain but you only do it once

fatchat commented 3 months ago

hi @Ishankoradia @kauxp where are we on this?

kauxp commented 3 months ago

I am working on the rest of the functions and will update you within a few days.

kauxp commented 3 months ago

hi @Ishankoradia @kauxp where are we on this?

I have removed all the functions that I found in the code base but I have not removed the following :

http://localhost:4200/proxy/deployments/ : there was more than one function with the same endpoint, so do I have to remove only the POST functionality ?

http://localhost:4200/proxy/blocks/dbtcore_edit/{wtype}/ : didn't find this end-point in the code base

http://localhost:4200/proxy/deployments/{deployment_id} : there was more than one function with the same endpoint, so do I have to remove only the PUT functionality ?

http://localhost:4200/proxy/blocks/airbyte/connection/byblockid/{block_id} : didn't find this end-point in the code base but there is one with the similar name /proxy/blocks/airbyte/connection/byblockid/{blockid} , should I have to remove this?

Let me know once this PR is ready to merge, I will squash all the commit messages into one for a better git history. Also could you please tag it as the C4GT community project.

fatchat commented 3 months ago

@kauxp i'll start reviewing now, would you resolve the conflicts please? they're probably from another PR we just merged

fatchat commented 3 months ago

hi @Ishankoradia @kauxp where are we on this?

I have removed all the functions that I found in the code base but I have not removed the following :

http://localhost:4200/proxy/deployments/ : there was more than one function with the same endpoint, so do I have to remove only the POST functionality ?

Please remove the POST as well as the PUT endpoints i.e. main.py:400 and main.py:436 as well as the service functions they call

http://localhost:4200/proxy/blocks/dbtcore_edit/{wtype}/ : didn't find this end-point in the code base

yes there are two in main.py starting at lines 273 and 291 resp.

http://localhost:4200/proxy/deployments/{deployment_id} : there was more than one function with the same endpoint, so do I have to remove only the PUT functionality ?

yes only the PUT

http://localhost:4200/proxy/blocks/airbyte/connection/byblockid/{block_id} : didn't find this end-point in the code base but there is one with the similar name /proxy/blocks/airbyte/connection/byblockid/{blockid} , should I have to remove this?

yes please, main.py:193

i'll create the C4GT issue and link this PR

kauxp commented 3 months ago

I have resolved the conflicts, can you verify this once and review the changes.

fatchat commented 3 months ago

there are no conflicts

waiting for you to remove post_dataflow, put_dataflow etc as requested in my last comment

kauxp commented 3 months ago

Hi @fatchat, I made the changes you suggested and squashed my commit messages into one. Please take a look and let me know if I left anything out.

fatchat commented 3 months ago

@kauxp something weird is happening... when i git pull i get

hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint:
hint:   git config pull.rebase false  # merge
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint:
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.

this means that main has some commits which are ahead of your branch

would you please git merge main and then push again?

kauxp commented 3 months ago

Hi, I tried fetching and merging but everything seems to be up to date, if this is not the case can you please elaborate on this issue.

fatchat commented 3 months ago

Hi, I tried fetching and merging but everything seems to be up to date, if this is not the case can you please elaborate on this issue.

ok i deleted the branch locally and pulled it again, that seems to have sorted it out

ran tests, all passed

will do another look-through and then merge