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

[AUTO-MERGE] Merge main into develop via merge-main-to-develop #1263

Merged
merged 7 commits into from
Feb 24, 2022

Conversation

idanov
Copy link
Member

@idanov idanov commented Feb 17, 2022

A new change in main cannot be merged into develop as part of the regular sync job, hence this PR. Please resolve the conflicts manually, and make sure to obtain 2 approvals once the builds pass.

IMPORTANT NOTICE

Please let CircleCI merge this PR automatically, with merge commit enabled.

…kedro into merge-main-to-develop

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
@SajidAlamQB SajidAlamQB requested a review from merelcht February 23, 2022 15:24
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! 👍

It's weird that the coverage is failing for tests/framework/cli/micropkg/conftest.py .. Firstly, I'm wondering if coverage should be run on test files at all (I raised this point in my PR for python 3.10 as well), but secondly, this conftest was copied right, so how come it was covered fine before? 🤔

@SajidAlamQB
Copy link
Contributor

SajidAlamQB commented Feb 23, 2022

LGTM! 👍

It's weird that the coverage is failing for tests/framework/cli/micropkg/conftest.py .. Firstly, I'm wondering if coverage should be run on test files at all (I raised this point in my PR for python 3.10 as well), but secondly, this conftest was copied right, so how come it was covered fine before? 🤔

I think it's because those lines were covered in test_pipeline.py an equivalent to this test file was not made for micropkg as it wasn't needed. More specifically when we ran the test test_catalog_and_params.

@merelcht
Copy link
Member

I think it's because those lines were covered in test_pipeline.py an equivalent to this test file was not made for micropkg as it wasn't needed. More specifically when we ran the test test_catalog_and_params.

Ah okay, so maybe the fixture can be cleaned up a bit?

Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

quick comments before final review (got 2 files left)

kedro/framework/cli/pipeline.py Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_package.py Outdated Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_package.py Outdated Show resolved Hide resolved
tests/framework/cli/micropkg/test_micropkg_package.py Outdated Show resolved Hide resolved
Signed-off-by: SajidAlamQB <90610031+SajidAlamQB@users.noreply.github.com>
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

LGTM!

setup(
name="{name}",
version="{version}",
description="Modular pipeline `{name}`",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I should've seen this before, but this shouldn't refer to modular pipelines.

@lorenabalan lorenabalan merged commit 35372a3 into develop Feb 24, 2022
@lorenabalan lorenabalan deleted the merge-main-to-develop branch February 24, 2022 17:09
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.

4 participants