Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from CircleCI to GitHub Actions #1057

Merged
merged 95 commits into from
Nov 2, 2021
Merged

Switch from CircleCI to GitHub Actions #1057

merged 95 commits into from
Nov 2, 2021

Conversation

HAEKADI
Copy link
Contributor

@HAEKADI HAEKADI commented Oct 1, 2021

Technical changes

  • Switch CI provider from CircleCI to GitHub Actions

To Do:

  • Test deploy
  • Add PyPI password and CircleCI token to GitHub Actions

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 HAEKADI changed the title GitHub actions Switch from CircleCI to GitHub actions Oct 1, 2021
@HAEKADI HAEKADI self-assigned this Oct 1, 2021
@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 1, 2021

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.

@HAEKADI HAEKADI changed the title Switch from CircleCI to GitHub actions Switch from CircleCI to GitHub Actions Oct 1, 2021
@bonjourmauko
Copy link
Member

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 HAEKADI force-pushed the github-actions branch 2 times, most recently from 38db8d1 to bb30986 Compare October 20, 2021 09:47
@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 25, 2021

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?

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Good!

.github/has-functional-changes.sh Show resolved Hide resolved
.github/is-version-number-acceptable.sh Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
@MattiSG MattiSG mentioned this pull request Oct 26, 2021
@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 26, 2021

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
Copy link
Member

MattiSG commented Oct 26, 2021

Thanks @HAEKADI! 😊
I updated the token to an “admin” scope, does it work now? 🙂

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 26, 2021

@MattiSG Permission denied comes up still 😕

@MattiSG
Copy link
Member

MattiSG commented Oct 26, 2021

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
Copy link
Member

MattiSG commented Oct 26, 2021

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

@bonjourmauko
Copy link
Member

Thanks for considering me for review @HAEKADI 😊 .

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

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 27, 2021

Thank you for the feedback @maukoquiroga ^^

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Oct 27, 2021

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

@MattiSG
Copy link
Member

MattiSG commented Oct 27, 2021

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
Copy link
Contributor Author

HAEKADI commented Oct 28, 2021

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
Copy link
Contributor Author

HAEKADI commented Oct 28, 2021

All that's left is to:

    • decide whether or not we're going to ignore changes to the tasks files;
    • update merge rules.

@HAEKADI HAEKADI requested a review from MattiSG October 28, 2021 15:44
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

🤩 🚢

.github/workflows/workflow.yml Show resolved Hide resolved
path: ${{ env.pythonLocation }}
key: build-${{ env.pythonLocation }}-${{ hashFiles('setup.py') }}-${{ github.sha }}
- name: Check NumPy typing against latest 3 minor versions
run: for i in {1..3}; do VERSION=$(${GITHUB_WORKSPACE}/.github/get-numpy-version.py prev) && pip install numpy==$VERSION && make check-types; done
Copy link
Member

Choose a reason for hiding this comment

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

Why is this implemented with a shell script instead of with the matrix strategy? 🙂

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 tried using the matrix strategy but it was much more complicated than I anticipated since it is a dynamic matrix build which is problematic.

To my understanding it would require, first, creating another job, with a for loop, to get the three Numpy versions to check, store them into a JSON format and then using a matrix strategy (in the job itself) go through the list and install one by one. I was not successful in doing so, but I did not try for very long.

In my opinion, it is a complicated implementation for a simple output that is executed much more elegantly with the existing job and in 30 seconds. If however you think there is a simpler way to do it, or if it's worth the time investment, I'm happy to do it 😉

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 will merge this PR for the time being. And if need be address this issue in a new one :)

Copy link
Member

Choose a reason for hiding this comment

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

IMHO this script is / will be useless once the matrix strategy is in place —if not done already.

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@MattiSG
Copy link
Member

MattiSG commented Oct 29, 2021

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

HAEKADI and others added 4 commits November 2, 2021 08:43
@HAEKADI HAEKADI merged commit 976c8c7 into master Nov 2, 2021
@HAEKADI HAEKADI deleted the github-actions branch November 2, 2021 11:21
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