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

Unexpected result on CircleCI at version number check #944

Closed
sandcha opened this issue Feb 5, 2020 · 5 comments
Closed

Unexpected result on CircleCI at version number check #944

sandcha opened this issue Feb 5, 2020 · 5 comments
Assignees
Labels
kind:ci Continuous ops, integration & deployment

Comments

@sandcha
Copy link
Collaborator

sandcha commented Feb 5, 2020

Hi there!

I really enjoy OpenFisca, but I recently encountered an issue.

Here is what I did:

I introduced changes to openfisca_core/simulation_builder.py.

Here is what I expected to happen:

I expected CircleCI to tell me that I might have introduced functional changes and so, that I should bump the version number.

Which means that I expected .circleci/is-version-number-acceptable.sh to fail.

Here is what actually happened:

CircleCI build_and_deploy step didn't fail. So I was allowed to merge the PR without bumping its version number.

Here is data (or links to it) that can help you reproduce this issue:

This other CircleCI build fails as expected when a functional change is introduced.

The issue might explain why this previous deployment failed on master. 🤔

Context

I identify more as a:

  • Developer (I create tools that use the existing OpenFisca code).
@Morendil
Copy link
Contributor

Morendil commented Feb 5, 2020

Your commit with changes to simulation_builder.py was in the same commit with changes to CHANGELOG.md - this isn't the usual case, we normally separate functional changes and version bumps into separate commits.

There is code in .circleci/is-version-number-acceptable.sh to detect that condition and still exit with an error if that is the case, I suspect that this code (rarely exercised) fails somehow. However I'm not able to reproduce the execution path that it must be taking to see the result we see (i.e. the specific sequence of messages echoed and a zero exit code).

@Morendil
Copy link
Contributor

Morendil commented Feb 7, 2020

I've been looking more closely at this and I'm not sure I understand what's going on; I wasn't looking at the right commits. The commit you are pointing to doesn't have changes to simulation_builder.py; its parent commit does (my comments above were based on that). And the "functional changes" message output does include simulation_builder.py. But that parent belonged to #925 so it's not clear why these changes were detected in the context of a build of #939… I'm feeling confused.

@Morendil
Copy link
Contributor

Morendil commented Feb 7, 2020

Upon rerunning (with SSH so I'd be able to investigate) https://circleci.com/gh/openfisca/openfisca-core/6100 fails in the expected way. Ditto on rerunning without SSH (in case that made a difference).

@Morendil
Copy link
Contributor

Morendil commented Feb 7, 2020

I suspect the underlying issue is our use of git diff-index which can be sensitive to timestamps; I'm seeing unexpected behaviour differences between running the command in a Docker container vs locally. It's possible git diff-index --cached would be more reliable and still do what we want, though I'm not expert enough in Git to be sure.

@bonjourmauko bonjourmauko added the kind:ci Continuous ops, integration & deployment label Apr 10, 2021
@bonjourmauko bonjourmauko self-assigned this Apr 10, 2021
@bonjourmauko
Copy link
Member

Hello! I think this has been solved since, I've tried here. I'm hence closing, but please do not hesitate to reopen this issue were you to experience the problem again in the future 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:ci Continuous ops, integration & deployment
Projects
None yet
Development

No branches or pull requests

3 participants