-
Notifications
You must be signed in to change notification settings - Fork 75
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
Consolidate build workflow #1040
Comments
Proposition 0: do nothingThe status quo is acceptable. It is not worth investing in changing something that works, the problems identified mostly concern maintainers, who are in a limited number and can learn to cope with that complexity and decrease it slowly when opportunities arise. |
Proposition 1: converge on a task managerAll workflow steps are declared in one place, and referenced elsewhere as needed. This can be achieved through
|
Proposition 2: consider GitHub Actions as the task managerCI is the main way all tests are executed, and is the most precise way to orchestrate the entire workflow, relying on a well documented syntax. With the migration to GitHub Actions (#1030), we have the opportunity to use
|
Proposition 3: CLI for users, task manager for contributorsThe CLI is maintained and possibly improved for users who simply install OpenFisca as a dependency, through its Web or Python API. At the same time, contributors are directed to a task manager. CI config barely orchestrates the tasks declared in the task manager.
|
|
I'm pretty much in favour of proposition 3 :) |
@maukoquiroga thanks for your enthusiasm! 😃 In order to minimise noise though, I encourage all participants to focus on using emoji reactions to express their preferences, and to use comments to add details and counter-propositions, as done above 🙂 Thank you, and sorry for being a killjoy 😉 |
Reminder: this RFC will close in 48 hours, unless the proposition set changes 🙂 Please cast your vote! |
Thanks @MattiSG! So to add details as what my perfect solution could look like (mostly proposition 3 but a bit of 1): Installing, building, etc.
The CLILet's call it
The task manager
ParallelisationGood old ## Launch all `openfica` tasks.
all: \
openfica-clean \
openfica-check-syntax-errors \
openfica-check-style \
openfica-check-types \
openfica-test \
;
openfica-%:
@openfica make.$* And then ✨ :
Like it?You can try a working prototype here 🥳 |
Thanks everyone for your participation! ConclusionProposition 3 is both the most consensual and the most supported, with 5 👍 and 0 👎 . On top, @HAEKADI's detailed exploration demonstrates that proposition 2 would be impossible to implement. Thus, Proposition 3: “CLI for users, task manager for contributors” is adopted. I'll lock this conversation in order to lock votes. Implementation choices will move to the open issues and PRs, and to new ones if some elements are not already covered 🙂 Survey learningsThe data from the survey contains some interesting learnings, which I'll highlight below. Feel free to examine it and share your own learnings! I suggest that contribution number 6 should be excluded from analysis as it contains several answers that are impossible (using the OpenFisca CLI to deploy and uninstall the package). Two things that I learned:
The distribution in usage shows that:
I believe this should make us adjust slightly the proposal in that the |
RFC: Consolidate build workflow
Context
There are currently four sources of truth for how to build, test and deploy an OpenFisca package:
.circleci/config.yml
, soon.github/workflows/config.yml
, cf. Switch from CircleCI to GitHub Actions #1030)..circleci/*.sh
, soon.github/*.sh
).Makefile
.Depending on each case, these complement (installing locally, testing both locally and in CI, packaging in CI), reference (
publish-git-tag.sh
from CI config,openfisca test
intest-api.sh
) or overlap (make check-style
vs.circleci/lint-changed-python-tests.sh
) each other.Problem
This setup raises the following issues:
make install
differs from the install result in CI, which callsmake build
instead), leading to inconsistent test results (e.g. build: test against packaged version #1037).make test
and contributorsopenfisca test
), leading to blocked PRs (e.g. Allow variable neutralization in YAML test files #1021 (comment)).Discussion
A synchronous discussion with @benjello @benoit-cty @sandcha @maukoquiroga @HAEKADI @MattiSG took place on 02/09/2021 on this topic.
Conclusions on that topic were that:
make
overopenfisca
as it is shorter, especially when multiple country packages are installed in one virtualenv (make test
vsopenfisca test --country-package openfisca_france tests
).Annex
Known workflow steps
The following table aims at summarizing all known declared workflow steps. It was established by @MattiSG on 03/09/2021, based on reading through Core, France and Country Template
Makefile
,.circleci/config.yml
and.circleci/*.sh
.pip install --editable
(optional)make deps
,make install
pip install openfisca_country_package
lint-changed-python-files.sh
,lint-changed-yaml-tests.sh
make check-syntax-errors check-style
make format-style
pytest && openfisca test
make test
openfisca test
make api
(in Core)make serve-local
(in Country Template)openfisca serve
git fetch && check_version_and_changelog.sh
check_version_and_changelog.sh
make build
make uninstall
,make clean
test-api.sh
publish-python-package.sh && publish-git-tag.sh
publish-python-package.sh && publish-git-tag.sh
make check-types
Usage survey
In parallel to this RFC, in order to identify which tasks might be removed altogether from the list of existing ones, and which destination is least likely to disrupt existing uses, a survey has been created by @MattiSG to better understand the current usages. You are strongly encouraged to participate to it: share your current workflow.
The results of the survey are publicly available.
Process
This RFC aims at finding the best solution to the problems identified above. Contributors are invited to vote on the propositions below, which will be exposed each in their own post, through emoji reactions (:+1: to support an option, :-1: to reject it, :eyes: to indicate having read it and not supporting nor rejecting it). Contributors are also invited to ask for clarifications, suggest improvements to the propositions if that could change their stance on them, and contribute their own if they believe they can offer significant improvements.
In order to contribute a proposition, please make clear whether it is a new one, in which case you should give it a new number (e.g. “proposition 5”), or if it builds upon an existing one to add some detail, in which case you should suffix the original number with a letter (e.g. “proposition 3A”).
This RFC will close after one week (7 ⨉ 24 hours) without changes to the proposition set. At that time, votes will be counted and the most consensual proposition will be implemented.
The text was updated successfully, but these errors were encountered: