Skip to content

SECURITY-47: Add GHA workflows#24

Merged
Xiangs18 merged 13 commits intodevelopfrom
dev-add_gha
Mar 18, 2025
Merged

SECURITY-47: Add GHA workflows#24
Xiangs18 merged 13 commits intodevelopfrom
dev-add_gha

Conversation

@Xiangs18
Copy link
Contributor

No description provided.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I'm assuming these are the same workflows we're using everywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: 3.9.19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why such a low version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the other repos where we have done the Mongo upgrade are using this version. I'm trying to make it consistent here. If you prefer 3.11.x, that's fine. I don't have a strong opinion on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember off the top of my head, but I think there were specific reasons why we used lower versions. If it's just as easy to go to a higher version go ahead and do it. Also make sure the test version and build version are aligned

Copy link
Contributor Author

@Xiangs18 Xiangs18 Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the build version to 3.9.19, 19/39 tests would fail. If we want to git them fixed, I prefer to do them in the next PR. If we upgrade python version to 3.11.x, we need more dep changes because of build error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leave it as 3.9 in that case, don't bother upgrading for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, so we need to create an issue for that as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it needs to be updated in the build in this PR - the build and tests need to be aligned

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@Xiangs18 Xiangs18 Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented out the mypy check to allow the tests to run. Let me know if you know how to fix 945b204

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience mypy isn't worth the trouble as long as you have reasonable tests. I like what it's trying to do but it seems too clunky (although I haven't tried it recently, maybe it's better now)

shell: bash
run: |
docker compose up -d
sleep 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yuck but ok, that's what's in travis.yml

Comment on lines +33 to +34
pip install -r requirements.txt
pip install -r dev-requirements.txt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make an issue to upgrade to pipenv or something later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#25

Comment on lines +9 to +10
coverage run --source=src/caching_service -m unittest discover src/test/caching_service
coverage report
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add a ticket to convert to pytest?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean just this line, or the entire test codebase, from unittest to pytest?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines. pytest is compatible with unittest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

converted to pytest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still using coverage to run the tests though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just realized that there hasn't been a post from codecov so it seems that's not working yet...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure; it seems like it cannot process the upload.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you setting the right format for the coverage report? https://github.com/kbase/cdm-task-service/blob/main/.github/workflows/test.yml#L76

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's what I thought. Let's see if it passes this time.

@Xiangs18 Xiangs18 mentioned this pull request Mar 14, 2025
@codecov
Copy link

codecov bot commented Mar 17, 2025

Welcome to Codecov 🎉

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 ☂️

@Xiangs18 Xiangs18 requested a review from MrCreosote March 17, 2025 22:31
Copy link
Member

@MrCreosote MrCreosote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @bio-boris do you want to review?

@Xiangs18
Copy link
Contributor Author

I'm curious why the Makefile requires docker compose run web sh scripts/run_tests.sh instead of just sh scripts/run_tests.sh.

@MrCreosote
Copy link
Member

I'm just guessing but I assume the docker file is installing test dependencies that may not exist on the system.

Copy link

@bio-boris bio-boris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@MrCreosote MrCreosote changed the title Add GHA workflows SECURITY-47: Add GHA workflows Mar 18, 2025
@Xiangs18 Xiangs18 merged commit 37ef46b into develop Mar 18, 2025
11 checks passed
@Xiangs18 Xiangs18 deleted the dev-add_gha branch March 18, 2025 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants