openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
168 stars 75 forks source link

Switch from CircleCI to GitHub Actions #1057

Closed HAEKADI closed 2 years ago

HAEKADI commented 2 years ago

Technical changes

To Do:

Once Openfisca-doc has migrated to GitHub Actions as well, the use of CircleCI API to update the doc should be changed as well.

HAEKADI commented 2 years ago

The test-core job fails if the build is not incorporated in the same job. I have no idea why. When running pytest alone, the yaml tests fail with a file not found error. Even with the build cache restored. I tried caching the entire home directory and it still fails.

The work around was to add the make build to the test-core job after restoring the env cache. It takes on average 23 extra seconds.

bonjourmauko commented 2 years ago

The test-core job fails if the build is not incorporated in the same job. I have no idea why. When running pytest alone, the yaml tests fail with a file not found error. Even with the build cache restored. I tried caching the entire home directory and it still fails.

The work around was to add the make build to the test-core job after restoring the env cache. It takes on average 23 extra seconds.

1048

HAEKADI commented 2 years ago

Hello @maukoquiroga! Since I rebased this PR the coverage went down to zero (for both make test-core and make test)with a CoverageWarning: No data was collected. (no-data-collected) warning. Which was not the case before the rebase πŸ€” Do you have any idea why that might be the case?

HAEKADI commented 2 years ago

I tested deploy and the package is indeed published. But the documentation update is still problematic due to permission issues.

Screenshot 2021-10-26 at 16 15 10

.

MattiSG commented 2 years ago

Thanks @HAEKADI! 😊 I updated the token to an β€œadmin” scope, does it work now? πŸ™‚

HAEKADI commented 2 years ago

@MattiSG Permission denied comes up still πŸ˜•

MattiSG commented 2 years ago

Mmh, I understand that I'd need to issue a personal token to get support for the API v1… I'm not so keen on that considering there is no scope limitation and I am admin on over 20 organisations. Could we consider using API v2 if it is not too costly? If that takes you more than one hour to put together, you can instead issue a personal token for yourself and send it to me in two pieces through side channels (e.g. Slack and SMS), I will then set it up πŸ™‚

MattiSG commented 2 years ago

(as a side note, the deploy job should fail if update-doc fails, so we know…)

bonjourmauko commented 2 years ago

Thanks for considering me for review @HAEKADI 😊 .

I barely know Github Actions so I did what I could.

HAEKADI commented 2 years ago

Thank you for the feedback @maukoquiroga ^^

HAEKADI commented 2 years ago

@MattiSG I upgraded to CircleCI API v2 and added a personal token that I sent you.

MattiSG commented 2 years ago

I upgraded to CircleCI API v2 and added a personal token that I sent you

I don't think we needed both πŸ˜‰ The current CIRCLECI_OPENFISCADOC_TOKEN should be valid for API v2. Could you please trigger a build and check that deployment works for you under these conditions? If not, I added the personal token you sent me under the CIRCLECI_V1_OPENFISCADOC_TOKEN secret. I will delete whichever one we don't need.

HAEKADI commented 2 years ago

I used both because according to the doc project tokens are not supported on API v2. I switched back to v1.1 because a trigger is needed to use v2 and I would have to modify the openfisca doc CircleCI config file in order to do so. As of now, the update doc step is functional with a personal token πŸ₯³ I think this should be documented for future use. Should I do it directly with a comment in the workflow file or somewhere else?

HAEKADI commented 2 years ago

All that's left is to:

    • [x] decide whether or not we're going to ignore changes to the tasks files;
    • [x] update merge rules.
MattiSG commented 2 years ago

I switched off all requested checks. Please notify admins on Slack to set GitHub Actions checks as mandatory once this has been merged πŸ˜ƒ