Fix/content dates for new version #365
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
k-burt-uch
force-pushed
the
fix/content-dates-for-new-version
branch
from
c7a63dd
to
f48fc26
Compare
k-burt-uch
dismissed stale reviews from BinamB and paulineribeyre
via
c94fe9e
|
Adding @Avantol13 to check the github actions CI |
|
|
||
| UnitTest: | ||
| name: Python Unit Test with Postgres | ||
| uses: uc-cdis/.github/.github/workflows/python_unit_test.yaml@master |
There was a problem hiding this comment.
the default script runs poetry run pytest -vv but in indexd we have coveralls set up and we want to report coverage updates, so i think we actually need a custom script that does this: https://github.com/uc-cdis/indexd/blob/2023.09/.travis.yml#L20-L23
There was a problem hiding this comment.
I'd actually be okay leaving this as-is, because if we want to add consistent coverage in the future, we could just modify the template. but we'd lose coverage reporting in the meantime. idk, I don't have a strong opinion either way
There was a problem hiding this comment.
It seems we've lost coverage reporting already, checking the last few closed PRs for indexd.
Checking this PR #361 and this one #360 there's no coveralls run. It seems the last one was Atharva's PR here: #359
I could be missing something, but it seems we've already lost coverage reporting so my vote is to add it to the template in a separate PR. Sound good @paulineribeyre ??
There was a problem hiding this comment.
I don't know why coveralls is not reporting as a check on those PRs, but you can see in coveralls that it did get the report, for example for #361:
But anyway i'm referring to the coverage badge we have on this repo's readme, more than to the PR checks. It would be nice to include the coveralls logic in the template, but since it's not there yet, i think removing it would be a regression and the coverage % on the repo readme would be inaccurate. Tbh i think it's probably not a heavy lift to add it, but if it is for some reason, we can file a ticket instead
There was a problem hiding this comment.
Ah, that makes sense. Given that https://github.com/uc-cdis/sheepdog#sheepdog has broken tags now I'll see what I can do about getting this working here and moving it to the template.
Reading the docs here https://github.com/marketplace/actions/coveralls-github-action it looks fairly straightforward but I might have to DM you with questions if I run into something I can't find in the coveralls config or guides.
Co-authored-by: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com>
k-burt-uch
force-pushed
the
fix/content-dates-for-new-version
branch
from
e9b7049
to
c32122e
Compare
.github/workflows/ci.yaml
Outdated
| with: | ||
| python-version: '3.9' | ||
| test-script: 'tests/ci_commands_script.sh' | ||
| Coveralls: |
There was a problem hiding this comment.
i don't think this will work because it's set up as a different "job", when it should be a new "step" in the same "job" (to use the report file generated by the pytest command). Unfortunately i don't think you can add a step to a job that's defined in a reusable workflow, so if you want to do it that way, it would have to be directly in the reusable workflow.
What i was suggesting was to add the coveralls command to tests/ci_commands_script.sh. But i think we'd need to pass the token to the test-script.
I see now that it's not as straightforward as i thought 😅
There was a problem hiding this comment.
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes