Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

[ci] move CI logic into shell scripts (fixes #114) #225

Merged
merged 11 commits into from
Nov 3, 2022
Merged

[ci] move CI logic into shell scripts (fixes #114) #225

merged 11 commits into from
Nov 3, 2022

Conversation

jameslamb
Copy link
Contributor

Fixes #114.

This PR proposes moving the logic currently contained in the project's CI (.circleci/config.yml) into shell scripts, for the reasons mentioned in #114:

  • reduced code duplication across jobs
  • reduced effort required to add, remove, and customize jobs
  • lower-friction local develoopment

Changes

  • moves commands for CI jobs into 2 shell scripts
    • .ci/setup.sh = install dependencies used by every CI job
    • .ci/test.sh = install hamilton's Python dependencies and run tests
  • adds docs in developer_setup.md explaining how to re-use these scripts to run tests in Docker locally during development
  • replaces uses of apt install with apt-get install
  • switches pyspark job to using a circleci/python image for consistency with all the other CI jobs + ease of use in development

How I tested this

Ran shellcheck on the new scripts.

shellcheck .ci/setup.sh
shellcheck .ci/test.sh

Ran yamllint on the new CircleCI config.

yamllint .circleci/config.yml

Tried running some of the project's tests with different combinations of TASK and PYTHON_VERSION.

For example:

Dask tests on Python 3.9 (click me)
docker run \
  --rm \
  --entrypoint="" \
  -v "$(pwd)":/opt/testing \
  --workdir /opt/testing \
  --env TASK=dask \
  -it circleci/python:3.9 \
  /bin/bash -c '.ci/setup.sh && .ci/test.sh'
main tests on Python 3.10 (click me)
docker run \
  --rm \
  --entrypoint="" \
  -v "$(pwd)":/opt/testing \
  --workdir /opt/testing \
  --env TASK=tests \
  -it circleci/python:3.10 \
  /bin/bash -c '.ci/setup.sh && .ci/test.sh'

Notes

References on re-using configuration in CircleCI configs:

Reference on why apt-get is preferred to apt for things like CI scripts:

Thanks for your time and consideration.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@jameslamb jameslamb changed the title WIP: [ci] move CI logic into shell scripts (fixes #114) [ci] move CI logic into shell scripts (fixes #114) Nov 1, 2022
@jameslamb jameslamb marked this pull request as ready for review November 1, 2022 04:24
Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

Looking good! :) just minor nits.

@jameslamb jameslamb requested a review from skrawcz November 1, 2022 21:10
@skrawcz
Copy link
Collaborator

skrawcz commented Nov 3, 2022

@jameslamb mind if I squash merge? Or do you want to rebase to resolve conflicts?

@jameslamb
Copy link
Contributor Author

@skrawcz sure, you can squash them! I just added you as a collaborator on my fork so you can do whatever you want there.


For your consideration... I personally feel like open source projects (at least here on GitHub) should just always use squash merging exclusively (one commit per PR, commit message = PR title) so you never have to think about commits on PRs.

Also means that if a PR gets behind the target branch, you can just git merge main to update it (instead of caring about rebasing).

That's been working great for us on LightGBM for years. Look how pretty this commit history is: https://github.com/microsoft/LightGBM/commits/master.

image

@jameslamb
Copy link
Contributor Author

And to be specific, I mean doing this in settings:

image

@skrawcz skrawcz merged commit f548b57 into stitchfix:main Nov 3, 2022
@jameslamb jameslamb deleted the ci-scripts branch November 3, 2022 17:27
@skrawcz
Copy link
Collaborator

skrawcz commented Nov 3, 2022

Thanks this @jameslamb !

@jameslamb
Copy link
Contributor Author

thanks so much for considering it!

@elijahbenizzy
Copy link
Collaborator

thanks so much for considering it!

Thanks for the contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] consolidate CI code into shell scripts
3 participants