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

Add CircleCI setup #4

Merged
merged 26 commits into from
Jan 25, 2022
Merged

Add CircleCI setup #4

merged 26 commits into from
Jan 25, 2022

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Jan 19, 2022

This PR is the complete set of changes required to have CCI builds for all plugins in kedro-plugins.

It has the changes from #2 with the addition of path-filtering which allows you to only run builds for the parts of the repo that have changed. I followed the steps here to get this to work.

I'll mark #2 as draft so you can use it as a reference how I came to the changes here, but I will only be merging be this PR.

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Still looking through the PR, will continue reviewing tomorrow! So far looks really great though 👍 👍

.circleci/continue_config.yml Outdated Show resolved Hide resolved
.circleci/continue_config.yml Outdated Show resolved Hide resolved
.circleci/continue_config.yml Show resolved Hide resolved
.circleci/continue_config.yml Show resolved Hide resolved
.circleci/continue_config.yml Show resolved Hide resolved
.circleci/continue_config.yml Outdated Show resolved Hide resolved
.circleci/continue_config.yml Outdated Show resolved Hide resolved
.circleci/continue_config.yml Outdated Show resolved Hide resolved
@antonymilne
Copy link
Contributor

antonymilne commented Jan 20, 2022

Overall this looks awesome! I was expecting something that was an MVP but this looks pretty close to an ideal solution, amazing work! Love the use of the matrix and parameters and all that - really pleased to see we can do per-directory selective workflows without too much faff. 🎉 In addition to my many minor comments, there's a few other things I think we should still consider (not necessarily in this PR).

pip freeze

Might be nice to add a pip freeze step like this so we can easily debug things.

Testing

By pushing to this branch and making sure only the right workflows run, making sure that the environment has the right requirements file installed (e.g. using pip freeze step), etc.

How to deal with pre-commit

Do the pre-commit hooks actually still work without the file being at the root of the repo? My guess based on this is maybe not 🤔

How to deal with Makefile

I'm not sure whether the Makefile should be per-project or at the repo root. It's definitely less repetitive the way you've done it, but when I'm working on a single plugin I would sort of expect the Makefile to live inside the directory for that individual plugin. Kind of like how each plugin has its own .pre-commit-config.yaml, pyproject.toml etc.

How to deal with parameters

3 boolean parameters like run-build-kedro-telemetry is how the CircleCI guide suggested doing this but actually I wonder if there's a neater way. Why can't we just use the name of the plugin directory as the parameter?

### config.yml
# 3-column, whitespace-delimited mapping. One mapping per
# line:
# <regex path-to-test> <parameter-to-set> <value-of-pipeline-parameter>
mapping: |
  kedro-telemetry/.* plugin-name telemetry
  kedro-docker/.* plugin-name docker
  kedro-airflow/.* plugin-name airflow

### continue_config.yml
parameters:
  plugin-name:
    type: enum
    enum: [telemetry, docker, airflow]

The advantage of this is you might then need a only single workflow that works for all 3 plugins instead of needing to switch between 3 different workflows dependent on the boolean:

workflow_name:
    jobs:
      - unit_tests:
          matrix:
            parameters:
              python_version: [ "3.6", "3.7", "3.8" ]
              plugin: <<pipeline.parameters.plugin-name>>
      - e2e_tests:
          matrix:
            parameters:
              python_version: [ "3.6", "3.7", "3.8" ]
              plugin: <<pipeline.parameters.plugin-name>>
      - win_build:
          matrix:
            parameters:
              python_version: [ "3.6", "3.7", "3.8" ]
              plugin: <<pipeline.parameters.plugin-name>>
      - lint:
          matrix:
            parameters:
              python_version: [ "3.8" ]
              plugin: <<pipeline.parameters.plugin-name>>

Related to this we are doing make plugin=$(plugin_name) command quite a bit. I wonder if this would be neater as an environment variable:

environment:
   plugin: <pipeline.parameters.plugin-name>>

And then you can call make command directly without needing to inject the plugin name into every make command individually. Or if the Makefiles do move into each individual directory maybe you'd just need to cd <<pipeline.parameters.plugin-name>> or change the working_directory to the plugin name parameter and then just run the make command.

Edit: just thought of a problem with this suggestion... What happens if I modify both telemetry and docker at the same time? I guess we need the 3 boolean variables so we can trigger workflows for multiple plugins simultaneously if required 🤔

@merelcht merelcht force-pushed the monorepo-cci-setup branch 12 times, most recently from c353405 to 5ec0b2d Compare January 20, 2022 12:44
merelcht and others added 24 commits January 24, 2022 10:42
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
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

4 participants