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

Fix Continuous Integration change detection #32526

Merged
merged 3 commits into from
Jan 10, 2020

Conversation

sureshc
Copy link
Contributor

@sureshc sureshc commented Dec 21, 2019

Description

Drone Continuous Integration builds skip unit tests when the target/base branch is staging-next because the detection of which source code directories have changed attempts to compare to origin/staging which is not present in the Drone container. The git command referencing the origin/staging branch raises an error because it has not been fetched. The best solution would probably be to compare the source code changes against origin/staging-next, but this comparison takes place a few layers down in the Continuous Integration rake call stack where information about what the target branch of the Pull Request that invoked the build is no longer available.

This change, instead, always fetches origin/staging. This assumes that most feature-branches off of the staging-next branch will trigger the same source code change detection when compared against staging.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

.drone.yml Outdated
# Always fetch staging branch because the change detection logic compares against the staging branch even when the base
# branch is staging-next:
# https://github.com/code-dot-org/code-dot-org/blob/8ea6ee6b63abd36e51c69d400eefdaa87154030a/lib/cdo/git_utils.rb#L104-L113
- git fetch --depth 100 origin staging
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it necessary to fetch to a depth?

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 don't see why it's necessary. My intent was to preserve the existing logic that fetches the target branch with a depth of 100 commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's needed to support the i18n sync process? 37120d8

Copy link

@uponthesun uponthesun Jan 6, 2020

Choose a reason for hiding this comment

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

We specify a depth to do a shallow clone, which is faster. We need a high enough depth so that the commit where the feature branch was cut from the base branch is included (I don't remember exactly which step needs this). The whole thing probably could be improved, haven't looked at it since early last year.

The i18n sync process just adds a lot of new commits to one feature branch, hence the increase to 100 in that commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's why it's necessary to fetch to a depth for the feature branch, but if we're just fetching the staging branch for the sake of change detection, shouldn't we only need to fetch the latest commit?

Choose a reason for hiding this comment

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

I'm not sure (don't know what the git command being run is), good point though. Worth testing.

@sureshc sureshc changed the base branch from staging to staging-next January 6, 2020 19:14
@uponthesun
Copy link

  • IIRC it's the unit tests that depend on this, not the UI tests. Probably safest just to make the change for both.
  • Does this increase the time needed to clone noticeably? If so, I think I'd prefer an approach where git_utils overrides its hardcoded default based on an environment variable.

@sureshc
Copy link
Contributor Author

sureshc commented Jan 8, 2020

Starting fresh with @uponthesun suggestion to use an environment variable to detect we're executing in a Drone build.

@sureshc sureshc requested a review from Hamms January 8, 2020 22:10
@sureshc sureshc merged commit 897ac92 into staging-next Jan 10, 2020
@sureshc sureshc deleted the fix-continuous-integration-change-detection branch January 10, 2020 17:49
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.

None yet

3 participants