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-3058] Make CircleCI not run all the jobs when just documentation is edited #1225

Merged
merged 19 commits into from
Feb 10, 2022

Conversation

lorenabalan
Copy link
Contributor

@lorenabalan lorenabalan commented Feb 8, 2022

Signed-off-by: lorenabalan lorena.balan@quantumblack.com

Description

It's annoying when a simple docs change is being held back by failing builds unrelated to the change, for instance because of a dependency upgrade that breaks the code build. This change should improve not only the wait time & costs, but also the developer experience overall.

Development notes

  1. Copy-pasted almost the entire contents of config.yml into continue_config.yml, with the following differences:
    • added build-docs and build-code to top-level parameters
    • renamed regular workflow to build_code and removed the build_docs and docs_linkcheck from its jobs (as well as its mandatory checks)
    • added build_docs workflow, where the jobs are build_docs and docs_linkcheck
    • updated the condition for the two workflows to a nested when instead of an unless
  2. Tested in [KED-3058] develop test - Make CircleCI not run all the jobs when just documentation is edited #1230 for develop. base-revision needs to be modified to point to develop on that branch.
  3. Found the suggestion here to use pipeline.git.branch parameter interesting but not sure if that's what we always want. It looks at changes within the branch, so if I committed a bunch of docs and code changes, then only docs changes, it will only run the docs build, not both. I believe this means that someone can have the tests failing, but if the last commit is docs and it all builds, they can still merge, which shouldn't be the case.

Resources:

Testing

CircleCI runs for a commit changing the docs:
Screenshot 2022-02-09 at 10 46 23

Only code:
Screenshot 2022-02-09 at 11 46 35

Both code and docs:
Screenshot 2022-02-09 at 11 46 48

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
lorenabalan added 2 commits February 8, 2022 14:35
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan self-assigned this Feb 8, 2022
lorenabalan added 10 commits February 9, 2022 10:35
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan lorenabalan marked this pull request as ready for review February 9, 2022 14:36
@lorenabalan lorenabalan requested review from antonymilne, merelcht and SajidAlamQB and removed request for yetudada February 9, 2022 14:37
# <regex path-to-test> <parameter-to-set> <value-of-pipeline-parameter>
mapping: |
docs/.* build-docs true
^((?!docs/).)*$ build-code true
Copy link
Contributor Author

@lorenabalan lorenabalan Feb 9, 2022

Choose a reason for hiding this comment

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

I know "anything but docs" might still be too generous, but it's equivalent to what we are currently doing and we can always further refine it in the future as we progress and learn more about / pay more attention to our pain points with this. I would still like changes in requirements.txt for instance to still trigger the code build, but stuff like LICENSE.md doesn't have to. I just didn't want to spend a whole lotta time on the perfect regex too early.

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.

Great PR! ⭐ Just a few minor comments/suggestions.

Some points to ponder, though:

  • should we still run the build_docs and link_check jobs when we build code, since docstrings generates the API docs? These aren't such slow or unreliable jobs, so I don't mind so much if they run unnecessarily, and they might sometimes catch an error in the docs - the important thing with this ticket was cutting out the unnecessary jobs when only the docs are changed.
  • if this does sound like a good idea then we could simplify things a bit by just having one parameter only-build-docs
  • actually, even if we don't do make this change we could simplify things a bit because (at least currently) you have build-docs = NOT build-code. So technically we could remove one of these and just use not in the when for the workflow

lorenabalan added 2 commits February 9, 2022 16:13
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
@lorenabalan
Copy link
Contributor Author

lorenabalan commented Feb 9, 2022

@AntonyMilneQB my first instinct is to prefer keeping the use of 2 parameters as I find it's more explicit and easier to read.

I need to think a bit more about the docstrings build. Personally it was something I could live with, rather than have code changes be blocked on linkchecker when it's irrelevant. I also don't know how I would split the jobs if we did this, without running the docs jobs twice, once as part of build_code and once as part of build_docs_only (I'm talking about the case where we have a PR that does both a code a change and a docs change).

@antonymilne
Copy link
Contributor

antonymilne commented Feb 9, 2022

I think you could do it by adding a not:<<pipeline.parameters.code_change>> to the build_docs_only: when? But yeah, I don't really mind. Happy with how it is and the docs not getting built every time.

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.

Great work, this is a huge improvement! 👏

I'm happy with this getting merged, but I do wonder whether we could end up in a situation where the docs aren't touched for a while and then suddenly someone makes a doc change and other things turn out to be broken because of code changes (e.g. through renaming of stuff) and this person has to clean up the mess (which could be an open source user, and they get sad by the mess we made 😿)

@lorenabalan
Copy link
Contributor Author

That's fair enough, I've made the change so that we run always code & docs builds, and only docs when there's no code changes.
@AntonyMilneQB thank you for the suggestion!! 🙌 I don't know how I couldn't think of that, my brain must've been fried yesterday. 🤦‍♀️

@lorenabalan lorenabalan merged commit 34915e1 into main Feb 10, 2022
@lorenabalan lorenabalan deleted the devops/ci-on-docs branch February 10, 2022 14:15
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
… is edited (kedro-org#1225)

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.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.

3 participants