search

elixir-cloud-aai / TESK

GA4GH Task Execution Service Root Project + Deployment scripts on Kubernetes
https://tesk.readthedocs.io
Apache License 2.0
39 stars 29 forks source link

feat: clean up & refactor majorly #171

Closed JaeAeich closed 4 months ago

JaeAeich commented 4 months ago

Description

fixes - #152 #153 #154 #155

JaeAeich commented 4 months ago

@uniqueg as discussed, naming src/tesk_core to tesk/service would make sense, but wouldn't that steer away from naming convention followed for released of modules, or will we keep the same name for the docker images (tesk-core) and just change the name in the code base (service)?

JaeAeich commented 4 months ago

everything seems to work post python version 3.11, I'll shift make name change, ie src->tesk at least, rest need to be discussed.

uniqueg commented 4 months ago

Also, why are Python 3.8, 3.9 and 3.10 failing, @JaeAeich?

On the other hand, it's not clear to me why we are even testing multiple Python versions, TESK is not a library. We pick one version and use that version, no need to support multiple Python versions...

JaeAeich commented 4 months ago

Also, why are Python 3.8, 3.9 and 3.10 failing, @JaeAeich?

On the other hand, it's not clear to me why we are even testing multiple Python versions, TESK is not a library. We pick one version and use that version, no need to support multiple Python versions...

I have no idea, lack of backwards compatibility ig, I haven't touched any code of tesk_core yet. Should I freeze python version to 3.11 and above (for pyproject.toml)? Will remove the other versions from API.

Also core seems to use kubernetes client v9 and rn its on v29 :face_in_clouds: , I'll definetely have to refactor core code as well in order to start to use same k8s client version on api.

JaeAeich commented 4 months ago

IG, changing the name of github actions workflow (to comply with other file name and repos) tripped it and its not showing tests, but all the tests passed. You can check it as well and I have simulated in on a clean ubuntu docker image just to be sure :)

uniqueg commented 4 months ago

IG, changing the name of github actions workflow (to comply with other file name and repos) tripped it and its not showing tests, but all the tests passed. You can check it as well and I have simulated in on a clean ubuntu docker image just to be sure :)

I think you need to update the test names in the branch protection rules. If you don't have the permissions (which I think you do) I'll do it when reviewing tomorrow.

JaeAeich commented 4 months ago

which I think you do

I don't have 'em! I don;t see branch protections rules in settings :cry: .

JaeAeich commented 4 months ago

Hey there are some TODOs that I'll take care in this PR itself, please ignore those and review the rest :)

uniqueg commented 4 months ago

Hey there are some TODOs that I'll take care in this PR itself, please ignore those and review the rest :)

They're here: https://github.com/elixir-cloud-aai/TESK/settings/branches

You need to edit the rule for branch main, then under "Require status checks to pass before merging" you need to use the search box to add the checks by name, e.g., "code-vulnerabilities" or "Semantic PR title".

Anyway, I have done that now in case you don't have access to that setting.

JaeAeich commented 4 months ago

Yeah I don't have the rights, but thanks for updating it :)

JaeAeich commented 4 months ago

We need codecov token, and helm CI is not running, we need to test that as well, I have broken that into multiple jobs to comply with a "consistency".

Apart from that since this PR has grown big enough why not discuss some extra things I found while going through other repos and etc etc.

uniqueg commented 4 months ago

We need codecov token, and helm CI is not running, we need to test that as well, I have broken that into multiple jobs to comply with a "consistency".

Apart from that since this PR has grown big enough why not discuss some extra things I found while going through other repos and etc etc.

  • Consider adding DCO check in CI.
  • Consider adding OSS licence compliance (license scan seems to be free 🤞 ) that shows, well what is says :), we can add its card to the readme as well with codecov, coverage.

Sure, feel free to add (low priority) issues for these :)

uniqueg commented 4 months ago

We need codecov token, and helm CI is not running, we need to test that as well, I have broken that into multiple jobs to comply with a "consistency".

Apart from that since this PR has grown big enough why not discuss some extra things I found while going through other repos and etc etc.

  • Consider adding DCO check in CI.
  • Consider adding OSS licence compliance (license scan seems to be free 🤞 ) that shows, well what is says :), we can add its card to the readme as well with codecov, coverage.

CODECOV_TOKEN is now set for this repo. Not sure what you need for the Helm CI, but I think we should merge this ASAP before it gets even more unwieldy. I propose that you remove the part that installs the chart for now (we can get it back from master later on), which is likely the one that fails at the moment? Or even remove the entire workflow.

codecov[bot] commented 4 months ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

JaeAeich commented 4 months ago

We're getting there, just a few small issues before we can merge.

However, sth is weird about your Git workflow. Several (but not all) files are shown here as completely new, even though there were just moved or a few lines were changed (e.g., pyproject.toml, some but not all test files moved from service to test_service). Try to figure out why that is and avoid it in the future, because it makes things hell to review and also you git blame will show you as the author of all of that code, even if you are not.

I don't think there is anything wrong with my workflow, the issue was that the code files were not formatted at all and had inconsistent spacing even between two words :sweat_smile: , plus fixing some minor issues to comply with Google's python coding guide like changing string concatenation to f string, these kind of small fixes were cluttered all around the code base. I believe that is the cause of this weird blame thing. But if it happens again I'll be sure to check into it :)

JaeAeich commented 4 months ago

@uniqueg please check if the badge I added is correct.