Publish Python 3 compatible packages and migrate to CircleCI 2.0 #41

fpagnoux · 2018-05-15T18:59:43Z

Deployment works for Python 2 and Python 3

MattiSG

Hard but necessary job! Thanks for handling it 😃 👍

MattiSG · 2018-05-15T21:55:05Z

deploy-if-version-bump.sh

+  fi
+fi
+
+if [ $PY_VERSION = "3" ]; then


So if the Python version is 3 then there is no functional changes check. Is that really the goal?

MattiSG · 2018-05-15T21:56:27Z

setup.py

@@ -7,7 +7,7 @@

 setup(
    name='OpenFisca-Country-Template',
-    version='3.0.2',
+    version='3.0.3',


Changelog and setup versions do not match.

I prefer the minor bump over the patch bump 🙂

MattiSG · 2018-05-15T21:57:29Z

.circleci/config.yml

+  country-template:
+    jobs:
+      - checkout
+      - dependencies:


What about suffixing with _python2 like it is done with _python3?

MattiSG · 2018-05-15T21:58:27Z

.circleci/config.yml

+          context: Py2
+          requires:
+            - tests
+            - tests_python3


This reads like “if I'm in Py2 context, run tests_python3” 🤔 Is this normal?

Got rid of contexts

MattiSG · 2018-05-15T21:58:44Z

.circleci/config.yml

+      - deploy_python3:
+          context: Py3
+          requires:
+            - tests


This reads like “if I'm in Py3 context, run tests (which is Python2)” 🤔 Is this normal?

Got rid of contexts

MattiSG · 2018-05-15T22:00:10Z

.circleci/config.yml

+          name: Deploy (if version bump)
+          command: |
+            . /tmp/venv/country-template/bin/activate
+            . deploy-if-version-bump.sh


Unless CircleCI does something special, the . is a non-readable equivalent of source. I recommend we use source over . for readability. I also believe that . deploy-if-version-bump.sh should not be sourced, but directly executed, and that it currently works only as a side-effect of sourcing.

MattiSG · 2018-05-15T22:00:51Z

.circleci/config.yml

+    steps:
+      - restore_cache:
+          keys:
+            - v1-checkout-{{ .Environment.CIRCLE_SHA1 }}


Such a key prevents the cache from having almost any use: only a rerun with cache would benefit from it.

I think it was only used to persist the files from one job to another. But yes, checksuming setup.py seems better.

MattiSG · 2018-05-15T22:01:35Z

.circleci/config.yml

+      - image: python:3.6
+
+  deploy: &deploy
+    working_directory: ~/country-template


I don't think you need to state this explicitly.

Indeed we don't

MattiSG · 2018-05-15T22:02:10Z

.circleci/config.yml

+      #       pip install --editable git+https://github.com/openfisca/openfisca-core.git@BRANCH_NAME#egg=OpenFisca-Core
+
+      - save_cache:
+          # PY_VERSION is set in the context


“the context” is ambiguous. I personally don't understand where this is coming from.

Got rid of that

MattiSG · 2018-05-15T22:06:09Z

I must say that I am a bit surprised and frustrated that my PR on CircleCI 2.0 migration (#40) has been lingering for over a week, and this one does essentially the same thing. I had already invested in this long, error-prone process, and now we have two very different implementations.

@fpagnoux I'll let you have a look at both implementations and assess if it would be easier to add Python3 support on top of #40 or integrate #40's syntax refinements in this PR.

How can we avoid such collaboration mishaps in the future?

fpagnoux · 2018-05-16T16:47:54Z

I updated the configuration to look more #40's one, which I find simpler.

To handle both Python 2 and Python 3, I went for duplication over complexity.

Reusing configuration means using complex YAML features that not many know about
Making one cache key by python version is tricky without complex and non straightforward console configuration (contexts)

@MattiSG let me know if that still seem too complicated for a package that aims to be cloned and re-used by many openfisca contributors.

fpagnoux · 2018-05-16T16:57:41Z

.circleci/deploy-python-3.sh

+
+set -e
+
+VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_core openfisca_web_api_preview"


@MattiSG I'm not sure about repeating this in 3 different files, as the whole continuous publishing relies on this variable to have the same values in the 3 files.

However, I didn't manage to share it. I tried to declare it in a separate shell file and to source it from each of the 3 scripts, but the source would not find the file (I think because relative paths in a script are always relative to where you launch the script, not where the script is).

If you know how to make it better, please proceed 🙂

I'll just extract that in a new script 😉

If you're curious, this is how I usually solve the problem.

MattiSG · 2018-05-17T02:42:48Z

.circleci/deploy-python-2.sh

+
+VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_core openfisca_web_api_preview"
+
+if git diff-index --quiet HEAD^ -- $VERSION_CHANGE_TRIGGERS ":(exclude)*.md"


We're only checking for changes in the last commit, not in the whole branch. Is that expected?

There is a subtlety here.

We are in the context of deployment. So there is not more feature branch involved: we are on master, and want to know if the new version of master brings any functional change compared to the last version of master. Hence the comparison with HEAD^.

This is different when we are testing a feature branch, and running check-version-bump.sh. In that case, we indeed want to compare our branch to the current version of master.

So we can't really share the whole detect-functional-changes.sh as is. I'll try to find a solution though.

MattiSG · 2018-05-17T02:44:35Z

.circleci/deploy-python-3.sh

+
+set -e
+
+VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_core openfisca_web_api_preview"


I'll just extract that in a new script 😉

If you're curious, this is how I usually solve the problem.

MattiSG · 2018-05-17T02:45:05Z

setup.py

@@ -16,7 +16,7 @@
    url='https://github.com/openfisca/openfisca-country-template',
    include_package_data = True,  # Will read MANIFEST.in
    install_requires=[
-        'OpenFisca-Core >= 23, < 24.0',
+        'OpenFisca-Core >= 23.1, < 24.0',


Any reason for that change here or is it just a knight? 😛

The changelog claims that we are making this package compatible with python 3.
We thus need to make sure we request a core version that is compatible with python 3.

MattiSG · 2018-05-17T02:46:12Z

.circleci/config.yml

+          name: Check version number has been properly updated
+          command: |
+            git fetch
+            ./.circleci/check-version-bump.sh


No need for the ./ when you're already giving a directory path.

I think you fixed it

MattiSG · 2018-05-17T02:46:33Z

.circleci/config.yml

+    docker:
+      - image: python:2.7.14
+    environment:
+      PYPI_USERNAME: openfisca-bot


Since this is a template, there should be a comment here about customising this value.

MattiSG · 2018-05-17T02:47:50Z

.circleci/config.yml

+      - build_python2
+      - build_python3
+      - deploy_python2:
+          requires:  # Only deploy if tests passed both with Python 2 and Python 3


This is quite readable in the config. This comment would add more value by explaining why we don't want to deploy unless both versions pass.

Ok, I thought it was unclear from a previous comment. Do you think we should not require the build_python2 for deploy_python3? I guess that we are on master, so technically the tests have already passed...

Removed the dependency, as it's not really necessary

I thought it would be because we don't really want to publish a new version for only one version: I wouldn't want to deal with version 23.1.2 existing in Python3 but not in Python2 and the other way around for 23.1.3 😅

MattiSG · 2018-05-17T02:57:31Z

.circleci/deploy-python-2.sh

-if ! git rev-parse `python setup.py --version` 2>/dev/null ; then
+
+if ./detect-functional-changes.sh
+then
    git tag `python setup.py --version`
    git push --tags  # update the repository version


I don't see the difference between Python2 and Python3 deployment beyond this initial tagging.
If indeed there is none, then I think the tagging should be done in CI (or in a script), and that the deployment should be a single script as it was. Right now, there is an apparent dependency of deployment of Python3 to deployment of Python2, which is both absurd and not guaranteed by CI; there is also unnecessary code duplication in the deployment scripts.

MattiSG · 2018-05-17T02:57:35Z

.circleci/deploy-python-2.sh

    git tag `python setup.py --version`
    git push --tags  # update the repository version
    python setup.py bdist_wheel  # build this package in the dist directory
    twine upload dist/* --username $PYPI_USERNAME --password $PYPI_PASSWORD  # publish
+    ssh -o StrictHostKeyChecking=no deploy-api@fr.openfisca.org  # Deploy the OpenFisca-France public API


I don't understand why this is in the country template 🤔

Excessive copy pasting

MattiSG · 2018-05-17T02:58:22Z

.circleci/detect-functional-changes.sh

+
+VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_country_template"
+
+if git diff-index --quiet origin/master -- $VERSION_CHANGE_TRIGGERS ":(exclude)*.md"


@fpagnoux Beware, this is not the same check as it was before I factored it: please double-check based on #41 (comment) that this is the proper check we want to apply here!

See reply on the said comment

fpagnoux · 2018-05-18T18:10:22Z

Tests ran:

A non functional change merged on master does not trigger a deployment: py2 py3
A functional change merged on master does trigger a deployment: py2 py3
A feature branch with no functional change does not need to update its version number: py2 py3
A feature branch with functional change that does not update the version number does not pass the tests: py2 py3
A feature branch with functional change that does not update the changelog does not pass the tests: py2 py3
A feature branch with functional change that updates the version number and the changelog does pass the tests: py2 py3

Now let's please get done with this 😓

MattiSG

Almost there… 😉 My only real problem is with the Python2 + Python3 dependency on deployment. We need to be 100% clear on the conditions we put on making a package available to the world.

MattiSG · 2018-05-19T04:23:55Z

.circleci/config.yml

+
+      - run:
+          name: Check for functional changes
+          command: if .circleci/detect-functional-changes.sh ; then circleci step halt ; fi


Why don't we just .circleci/detect-functional-changes.sh? Shouldn't deployment fail?

Alright I understood. No, it should not fail, it should be green but not do anything. Gotcha.

MattiSG · 2018-05-19T04:26:27Z

.circleci/config.yml

+      - build_python3
+      - deploy_python2:
+          requires:
+            - build_python2


I don't think we want to publish a Python2 package that does not pass Python3 builds. How would we manage a 23.1.2 tag that got published in Python2 but failed in Python3? We would then have to publish a 23.1.3 tag that would be available in both environments… So all version numbers could co-exist differently in each language version 😕

OK, I misinterpreted a previous comment.

MattiSG · 2018-05-19T04:27:38Z

.circleci/detect-functional-changes.sh

+
+VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_country_template"
+
+if [[ "$CIRCLE_BRANCH" == "master" ]]


No need for quotes there 😉

MattiSG · 2018-05-19T04:47:41Z

.circleci/publish-git-tag.sh

@@ -0,0 +1,4 @@
+#! /usr/bin/env bash


When you're not using bash-specific instructions, sh is safer: bash is an implementation with extensions while sh is an interface.

I'd add that usually sh is just a pointer to bash, but not always.

MattiSG · 2018-05-19T05:28:23Z

Tests ran: […]

I reviewed all links and agree with all the tests. Thank you so much for this list of links, it has proven extremely useful. It is also very reassuring to see how all possible cases have been covered 👏

fpagnoux · 2018-05-21T16:29:02Z

.circleci/detect-functional-changes.sh

-then LAST_MASTER_VERSION="HEAD^"
-else LAST_MASTER_VERSION="origin/master"
-fi
+last_tagged_commit=`git describe --tags --abbrev=0 --first-parent`  # --first-parent ensures we don't follow tags not published in master through an unlikely intermediary merge commit


Not sure I fully understand the motive of the change.

Can you give a concrete example of a situation where the previous implem would not do its job?

One risk I can see here is if for some reason a tag is published in the middle of the feature branch. Then the script would not compare the feature branch to the last published version, but to that tag.

This change has the technical benefit of working in both cases without needing specific logic based on the branch name.

In terms of logic, it makes the assumption that all tagged commits are published. The previous implementation made the assumption that all commits on master are published. I am more confident that no accidental tagging will happen than no commits on master can accidentally happen.
In the same way, if we want to unpublish a version (or mark a version as unpublished), we now simply have to remove a tag. Removing a commit on master is harder 😉

If you want added rigidity (which would go beyond what was already the case), I'm happy to add a check for the tag to be x.y.z, preventing preversions from being taken into account 🙂

Ok, fair enough.

I was thinking that under our workflow, accidental commits on master are pretty rare, if not inexistent, but we can't assume that it will be the same for all re-users.

Let's merge this as-is, and see if we need more refinement later.

under our workflow, accidental commits on master are pretty rare

Totally agree! It just happens that I believe the semantics of “this version was published” are more closely aligned with “this version is tagged” than “this version was merged into master”. Thanks to all the automation we've made, this is luckily all aligned, but I felt this implementation was both clearer and shorter 😉

fpagnoux · 2018-05-21T16:32:34Z

Changed the workflow so that we deploy only if the two builds have passed.

MattiSG · 2018-05-22T00:29:46Z

I'm changing the required CI checks to allow for merging.

MattiSG · 2018-05-22T00:30:33Z

Done.

Do not rely on master merge commits to necessarily have been tagged and published: if there is an issue there, we would enter a cycle of failing deployments

If none are detected, there will still be no output

fpagnoux · 2018-05-22T00:45:56Z

Pew, that was painful 😅

bonjourmauko · 2018-05-30T14:03:13Z

Wonderful thanks !

Publish py3 versions in CI & switch to Circle v2

7e99528

fpagnoux requested review from Anna-Livia and sandcha May 15, 2018 18:59

Don't try to publish the tag twice

9a13841

fpagnoux force-pushed the python3 branch from 58c5831 to 9a13841 Compare May 15, 2018 19:00

MattiSG requested changes May 15, 2018

View reviewed changes

fpagnoux commented May 16, 2018

View reviewed changes

fpagnoux requested a review from MattiSG May 16, 2018 16:57

fpagnoux mentioned this pull request May 16, 2018

Fixup py3 openfisca/openfisca-core#662

Merged

MattiSG requested changes May 17, 2018

View reviewed changes

MattiSG reviewed May 17, 2018

View reviewed changes

bonjourmauko assigned fpagnoux May 18, 2018

bonjourmauko added flow:team:doing labels May 18, 2018

bonjourmauko changed the title ~~Publish Python 3 packages~~ Publish Python 3 compatible packages May 18, 2018

bonjourmauko mentioned this pull request May 18, 2018

Make openfisca-core compatible with Python 3 openfisca/openfisca-core#630

Closed

6 tasks

fpagnoux force-pushed the python3 branch 3 times, most recently from 4f95107 to bb48902 Compare May 18, 2018 18:01

fpagnoux requested a review from MattiSG May 18, 2018 18:10

fpagnoux mentioned this pull request May 18, 2018

Fix CircleCI configuration openfisca/openfisca-core#666

Closed

MattiSG mentioned this pull request May 19, 2018

Migrate to CircleCI 2.0 #40

Closed

MattiSG changed the title ~~Publish Python 3 compatible packages~~ Publish Python 3 compatible packages and migrate to CircleCI 2.0 May 19, 2018

MattiSG requested changes May 19, 2018

View reviewed changes

fpagnoux commented May 21, 2018

View reviewed changes

fpagnoux requested a review from MattiSG May 21, 2018 16:31

fpagnoux removed request for sandcha and Anna-Livia May 21, 2018 16:31

MattiSG approved these changes May 22, 2018

View reviewed changes

fpagnoux and others added 12 commits May 21, 2018 20:42

Bump version number

f989ab3

Merge conf with #40's suggestion

e1e9f3d

Remove duplication in functional changes detection

e1e4598

Minor simplification of CircleCI syntax

78c7ab4

Add comment about Pypi username

006273d

Make script work in test and deploy context

5846f05

Refactor deployment

9c2153d

Bypass version check on master

96d7dae

Use relative path

e959284

Don't publish a tag in Py3

9c09dd0

Increase resilience of last tagged version search

266dd14

Do not rely on master merge commits to necessarily have been tagged and published: if there is an issue there, we would enter a cycle of failing deployments

Give a short list of functional changes

86886c4

If none are detected, there will still be no output

fpagnoux force-pushed the python3 branch from b99b907 to 86886c4 Compare May 22, 2018 00:44

fpagnoux merged commit 79c9572 into master May 22, 2018

fpagnoux deleted the python3 branch May 22, 2018 00:45

fpagnoux removed the flow:team:doing label May 22, 2018

MattiSG mentioned this pull request May 22, 2018

Return an exit code on functional changes detect #44

Merged

bonjourmauko added kind:solution and removed kind:improvement labels Jul 23, 2018


		set -e

		VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_core openfisca_web_api_preview"


		VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_core openfisca_web_api_preview"

		if git diff-index --quiet HEAD^ -- $VERSION_CHANGE_TRIGGERS ":(exclude)*.md"


		VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_country_template"

		if git diff-index --quiet origin/master -- $VERSION_CHANGE_TRIGGERS ":(exclude)*.md"


		VERSION_CHANGE_TRIGGERS="setup.py MANIFEST.in openfisca_country_template"

		if [[ "$CIRCLE_BRANCH" == "master" ]]

Publish Python 3 compatible packages and migrate to CircleCI 2.0 #41

Publish Python 3 compatible packages and migrate to CircleCI 2.0 #41

Conversation

fpagnoux commented May 15, 2018 • edited by bonjourmauko Loading

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiSG commented May 15, 2018

fpagnoux commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented May 18, 2018

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiSG commented May 19, 2018

fpagnoux May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux commented May 21, 2018

MattiSG commented May 22, 2018

MattiSG commented May 22, 2018

fpagnoux commented May 22, 2018

bonjourmauko commented May 30, 2018

fpagnoux commented May 15, 2018 •

edited by bonjourmauko

Loading

fpagnoux May 18, 2018 •

edited

Loading

fpagnoux May 21, 2018 •

edited

Loading