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

[KED-2556] Add CircleCI config #24

Merged
merged 23 commits into from
May 10, 2021
Merged

[KED-2556] Add CircleCI config #24

merged 23 commits into from
May 10, 2021

Conversation

lorenabalan
Copy link
Contributor

@lorenabalan lorenabalan commented Feb 25, 2021

Long-overdue enabling of CI.
Had to dig out https://github.com/quantumblacklabs/private-kedro/issues/790 to make sense in my head what we're doing..
kedro master should be in sync with starters master.

  • The CI output is quite hard to read if something does go wrong, there's a lot of scrolling but I'm not sure what's the best way to split these checks. Opinions and suggestions welcome.
  • I am hoping we will never need to create and maintain a starter called features...

TODO:

  • Remove features from each starter folder.
  • Include astro-iris starter in e2e testing.

@lorenabalan lorenabalan self-assigned this Feb 25, 2021
@lorenabalan lorenabalan force-pushed the fix/add-ci branch 3 times, most recently from f9b1978 to e827d31 Compare May 5, 2021 14:39
@lorenabalan lorenabalan changed the title Add CircleCI config [KED-2556] Add CircleCI config May 5, 2021
@lorenabalan lorenabalan marked this pull request as ready for review May 6, 2021 11:52
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Just some minor questions 🙂

features/lint.feature Show resolved Hide resolved
build:
jobs:
- security_scan
- e2e_38
Copy link
Member

Choose a reason for hiding this comment

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

Should these tests be run on Windows as well?

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 that's a good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting windows to work was really fiddly so I'll defer it to a separate ticket, and reap the benefits of having some CI working in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, that sounds good!

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.

This is awesome, I'm really pleased we have it 💯 Just a few comments/ideas/questions but generally looks great.

  1. (Not for this PR) Should we have a develop branch for kedro-starters? This could have analogous CI checks against kedro's develop branch? So we can continue developing and testing against 0.18 starters (e.g. following breaking changes to ConfigLoader, I can no longer work off kedro's develop and make a new project from a starter, only an empty kedro project).

  2. Possible extra behave tests to run: kedro ipython loads up ok; kedro test runs?

  3. Where is I have installed the Kedro project's dependencies defined? Do we need to be running each test in its own environment to make sure that kedro install for one starter doesn't interfere with another?

In terms of splitting things up, just thinking out loud here... I guess we have a 3 dimensional space here of environment (e.g. Python 3.7) x starter (e.g. spaceflights) x feature (e.g. lint). For CircleCI it makes sense to split up builts by environment, so then the only option is whether they should be broken down further, either by starter or by feature. Given the current grouping is done by feature, that would seem like the natural way to split it. i.e. you would have one build for each of:

  • Python 3.7, behave lint
  • Python 3.7, behave run
  • Python 3.8, behave lint
  • Python 3.8, behave run
  • etc.

I think it's possible to generate this sort of matrix of builds on CircleCI. But I don't think it's too bad just having them grouped as they currently are.

@antonymilne
Copy link
Contributor

antonymilne commented May 7, 2021

I guess the alternative way of grouping would be to group feature files by starter rather than sort of test. Maybe it's even possible to just make one such templated file and then provide the starter name as an argument:

Feature: Test starter {starter_name}

  Scenario: Run a Kedro project created from {starter_name}
    ...
  
  Scenario: Lint {starter_name}
    ...

and then do behave --args:starter_name=spaceflights. No idea whether that's possible with behave though, and it's not as explicit as the current setup.

@lorenabalan
Copy link
Contributor Author

lorenabalan commented May 7, 2021

  1. (Not for this PR) Should we have a develop branch for kedro-starters? This could have analogous CI checks against kedro's develop branch? So we can continue developing and testing against 0.18 starters (e.g. following breaking changes to ConfigLoader, I can no longer work off kedro's develop and make a new project from a starter, only an empty kedro project).

Mm in the past we just had a separate branch 0.17.0 that we tested manually with on kedro side, and the requirements were updated on the 0.17.0 branch to look at kedro develop. Personally I like that approach better just cause it keeps the CI setup super simple and readable. Having different workflows per branch and different requirements per branch feels a bit like an overkill to me for an uncomplicated repo. And tbf, the develop changes are already better tested than they used to, since we have the test_starter in the kedro repo, so the changes on kedro-starter would just have to mirror those in test_starter.

  1. Possible extra behave tests to run: kedro ipython loads up ok; kedro test runs?

I kinda prefer the tests to only check the critical workflow kedro run and any minor workflow can be fixed at any point with re-tagging a different commit - keeps the CI checks faster too. Plus, kedro test is not that important and kedro ipython will go away when we move to the iPython extension.

  1. Where is I have installed the Kedro project's dependencies defined? Do we need to be running each test in its own environment to make sure that kedro install for one starter doesn't interfere with another?

I thought this was already happening but I'll double check.

And yeah I thought about splitting by lint_py37, run_py37 etc, might just change it to that for clarity.

@lorenabalan lorenabalan force-pushed the fix/add-ci branch 3 times, most recently from 1b565f3 to 0fdf5fb Compare May 10, 2021 09:52
lorenabalan added 2 commits May 10, 2021 13:37
This reverts commit 481ba80.
This reverts commit e3bf336.
@lorenabalan lorenabalan merged commit 81b5e07 into master May 10, 2021
@lorenabalan lorenabalan deleted the fix/add-ci branch May 10, 2021 12:50
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