-
Notifications
You must be signed in to change notification settings - Fork 15
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 #115
Conversation
@MattiSG @maukoquiroga The old CI did not include a linting job #114, when adding it to the new CI Is there a consensus on whether one or both should be used? |
Unless they do different things, I am in favour of having only one linter, to make linting faster and more deterministic. @HAEKADI didn't you mention |
We did indeed 🙂 Now, if we have to reconsider the current way of linting, I just wanted to throw this in 😉 |
Hi !
We have agreed on the rules to encforce, that's why you will find them in the country and extension templates, yet still not implemented them in openfisca/openfisca-core#960 .
What they do differently is hang indents ( Other than that, I prefer whatever comes closes to the rules we have agreed upon (that excludes Specific example: in OpenFisca, a
Haven't heard of that one, I'll take a look. The minimal set of style rules for me is the one in the setup.cfg, well the exceptions:
Beyond that, I'm OK with whatever is faster and more deterministic as @MattiSG said. Update
|
Thank you for the clarification @maukoquiroga! I started implementing |
I don't have a clear view on latest discussions in the French community, but for the historical record
I can relate to that, see openfisca/openfisca-core#960 and https://github.com/openfisca/openfisca-france/issues?q=style+author%3Amaukoquiroga. |
I see. Maybe it's time to put pen to paper and have a clear definition of linting rules across all repos. I see you already started working on it @maukoquiroga with openfisca/openfisca-core#960. |
That would be great!!! See one of the discussions on I've started https://github.com/openfisca/openfisca-core/blob/master/STYLEGUIDE.md and I'm happy to help if you decide to continuing efforts on style coherence —which I think is a great idea. It is an idea that has been going on and proposed by some members. See https://github.com/openfisca/country-template/pull/111/files for example, beyond the actual implementation ( |
I see. There are certainly many ways to go about doing that. |
If I understand correctly,
However, in openfisca/openfisca-core#1040, we decided to focus on consolidating tasks in a task manager, not in CI. Thus, I understand that if we want to call both I also see in aaea4dd that the so-called “linting scripts” were removed, but these scripts do more than just activate linting: most of all, they select the subset of files to be linted in order to accelerate the process. I am thus not sure this should just be removed. |
As discussed this morning with @HAEKADI, based on the conclusions from openfisca/openfisca-core#1040, let's focus this PR on being isofunctional and moving to GitHub Actions without changing the services. |
I removed super-linter. |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make a search for circleci
in the entire repo, in order to make sure that there are no more references to it, and update the existing ones accordingly.
For example, the bootstrap script is missing an update 🙂
a5da1db
to
045915f
Compare
setup.py
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
setup( | |||
name = "OpenFisca-Country-Template", | |||
version = "3.12.9", | |||
version = "3.12.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of the Template, I believe that switching from CircleCI to GitHub Actions should be a minor, not a patch 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay :) so 3.13.0
?
.github/workflows/workflow.yml
Outdated
- name: Build package | ||
if: steps.restore-build.outputs.cache-hit != 'true' | ||
run: make build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't that leave the built package in its very first state, never updating the cache? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. We did encounter the problem in Openfisca-France
. I opened a PR to solve it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will integrate the changes here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow name is not updated when the bootstrap script is executed: the workflow.yml
file still contains name: Country Template
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally entirely on macOS 11.6 with an M1 processor, the bootstrap script seems to work well, but installing fails with ModuleNotFoundError: No module named 'Cython'
when trying to install numexpr 2.7.3
.
Since the tests pass, I assume this is related to my local setup and will approve this PR. I would appreciate a double check on this issue, as well as handling the macOS-specific comment 🙂
Looking forward to this landing!
ec21892
to
2eba652
Compare
Related to openfisca/openfisca-core/#1030
Related to openfisca/openfisca-core/#1027
Related to openfisca/openfisca-france/#1663
Closes #114
GitHub Actions