-
Notifications
You must be signed in to change notification settings - Fork 903
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 an option for kedro new to skip telemetry #3701
Changes from 10 commits
90fcd26
8d0d13b
a8b0036
482d093
6504100
09f174b
8917e85
2b3b046
583e5e7
f8254d0
fb5a876
c2c3906
650f37c
19ac1e7
ecc3780
329b05f
31089ee
39b9474
8733b2a
6c4ce42
c11f197
b34539c
3ee92fa
ef827ef
575929d
603c894
f9bcd70
29184db
51265e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,11 @@ | |
""" | ||
EXAMPLE_ARG_HELP = "Enter y to enable, n to disable the example pipeline." | ||
|
||
TELEMETRY_ARG_HELP = """Allow or not allow Kedro to collect usage analytics. | ||
We cannot see nor store information contained into a Kedro project. Opt in with "yes" | ||
and out with "no". | ||
""" | ||
Comment on lines
+70
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will take some reference from https://github.com/kedro-org/kedro-plugins/tree/main/kedro-telemetry, but probably get advice from @astrojuanlu since we need to be careful about the wordings. We did collect some Project information, such as number of pipelines, datasets, but they are anonymous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point, I will let him know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would copy the definition from the regex function as it's more descriptive:
|
||
|
||
|
||
@define(order=True) | ||
class KedroStarterSpec: | ||
|
@@ -261,14 +266,16 @@ def starter() -> None: | |
@click.option("--tools", "-t", "selected_tools", help=TOOLS_ARG_HELP) | ||
@click.option("--name", "-n", "project_name", help=NAME_ARG_HELP) | ||
@click.option("--example", "-e", "example_pipeline", help=EXAMPLE_ARG_HELP) | ||
@click.option("--telemetry", "-tc", "telemetry_consent", help=TELEMETRY_ARG_HELP) | ||
def new( # noqa: PLR0913 | ||
config_path: str, | ||
starter_alias: str, | ||
selected_tools: str, | ||
project_name: str, | ||
checkout: str, | ||
directory: str, | ||
example_pipeline: str, # This will be True or False | ||
example_pipeline: str, | ||
telemetry_consent: str, | ||
**kwargs: Any, | ||
) -> None: | ||
"""Create a new kedro project.""" | ||
|
@@ -280,6 +287,7 @@ def new( # noqa: PLR0913 | |
"checkout": checkout, | ||
"directory": directory, | ||
"example": example_pipeline, | ||
"telemetry_consent": telemetry_consent, | ||
} | ||
_validate_flag_inputs(flag_inputs) | ||
starters_dict = _get_starters_dict() | ||
|
@@ -343,7 +351,13 @@ def new( # noqa: PLR0913 | |
template_path=template_path, | ||
) | ||
|
||
_create_project(project_template, cookiecutter_args) | ||
if telemetry_consent is not None: | ||
_validate_input_with_regex_pattern("yes_no", telemetry_consent) | ||
telemetry_consent = ( | ||
"true" if _parse_yes_no_to_bool(telemetry_consent) else "false" | ||
) | ||
|
||
_create_project(project_template, cookiecutter_args, telemetry_consent) | ||
|
||
# If not a starter, print tools and example selection | ||
if not starter_alias: | ||
|
@@ -876,7 +890,9 @@ def _validate_range(start: Any, end: Any) -> None: | |
return selected | ||
|
||
|
||
def _create_project(template_path: str, cookiecutter_args: dict[str, Any]) -> None: | ||
def _create_project( | ||
template_path: str, cookiecutter_args: dict[str, Any], telemetry_consent: str | None | ||
) -> None: | ||
"""Creates a new kedro project using cookiecutter. | ||
|
||
Args: | ||
|
@@ -893,6 +909,10 @@ def _create_project(template_path: str, cookiecutter_args: dict[str, Any]) -> No | |
|
||
try: | ||
result_path = cookiecutter(template=template_path, **cookiecutter_args) | ||
|
||
if telemetry_consent is not None: | ||
with open(result_path + "/.telemetry", "w") as telemetry_file: | ||
telemetry_file.write("consent: " + telemetry_consent) | ||
except Exception as exc: | ||
raise KedroCliError( | ||
"Failed to generate project when running cookiecutter." | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1562,3 +1562,119 @@ def test_convert_tool_short_names_to_numbers_with_duplicates(self): | |
selected_tools = "lint,test,tests" | ||
result = _convert_tool_short_names_to_numbers(selected_tools) | ||
assert result == ["1", "2"] | ||
|
||
|
||
@pytest.mark.usefixtures("chdir_to_tmp") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving the tests related to tools to their own file and maybe moving auxiliary functions to their own file as well would probably solve half ot it. |
||
class TestTelemetryCLIFlag: | ||
@pytest.mark.parametrize( | ||
"input", | ||
["yes", "YES", "y", "Y", "yEs"], | ||
) | ||
def test_flag_value_is_yes(self, fake_kedro_cli, input): | ||
result = CliRunner().invoke( | ||
fake_kedro_cli, | ||
[ | ||
"new", | ||
"--name", | ||
"New Kedro Project", | ||
"--tools", | ||
"none", | ||
"--example", | ||
"no", | ||
"--telemetry", | ||
input, | ||
], | ||
) | ||
|
||
repo_name = "new-kedro-project" | ||
assert result.exit_code == 0 | ||
|
||
telemetry_file_path = Path(repo_name + "/.telemetry") | ||
assert telemetry_file_path.exists() | ||
|
||
with open(telemetry_file_path) as file: | ||
file_content = file.read() | ||
target_string = "consent: true" | ||
|
||
assert target_string in file_content | ||
_clean_up_project(Path("./" + repo_name)) | ||
|
||
@pytest.mark.parametrize( | ||
"input", | ||
["no", "NO", "n", "N", "No"], | ||
) | ||
def test_flag_value_is_no(self, fake_kedro_cli, input): | ||
result = CliRunner().invoke( | ||
fake_kedro_cli, | ||
[ | ||
"new", | ||
"--name", | ||
"New Kedro Project", | ||
"--tools", | ||
"none", | ||
"--example", | ||
"no", | ||
"--telemetry", | ||
input, | ||
], | ||
) | ||
|
||
repo_name = "new-kedro-project" | ||
assert result.exit_code == 0 | ||
|
||
telemetry_file_path = Path(repo_name + "/.telemetry") | ||
assert telemetry_file_path.exists() | ||
|
||
with open(telemetry_file_path) as file: | ||
file_content = file.read() | ||
target_string = "consent: false" | ||
|
||
assert target_string in file_content | ||
_clean_up_project(Path("./" + repo_name)) | ||
|
||
def test_flag_value_is_invalid(self, fake_kedro_cli): | ||
result = CliRunner().invoke( | ||
fake_kedro_cli, | ||
[ | ||
"new", | ||
"--name", | ||
"New Kedro Project", | ||
"--tools", | ||
"none", | ||
"--example", | ||
"no", | ||
"--telemetry", | ||
"wrong", | ||
], | ||
) | ||
|
||
repo_name = "new-kedro-project" | ||
assert result.exit_code == 1 | ||
|
||
assert ( | ||
"'wrong' is an invalid value for example pipeline. It must contain only y, n, YES, or NO (case insensitive)." | ||
in result.output | ||
) | ||
|
||
telemetry_file_path = Path(repo_name + "/.telemetry") | ||
assert not telemetry_file_path.exists() | ||
|
||
def test_flag_is_absent(self, fake_kedro_cli): | ||
result = CliRunner().invoke( | ||
fake_kedro_cli, | ||
[ | ||
"new", | ||
"--name", | ||
"New Kedro Project", | ||
"--tools", | ||
"none", | ||
"--example", | ||
"no", | ||
], | ||
) | ||
|
||
repo_name = "new-kedro-project" | ||
assert result.exit_code == 0 | ||
|
||
telemetry_file_path = Path(repo_name + "/.telemetry") | ||
assert not telemetry_file_path.exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is same question for
example_pipeline
, is there a reason why we cannot simplify this to--skip-telemetry
, when not used it just prompt as usual, otherwise just default to No? I am sure no one want to type out--telemetry no
since it's redundant.It's also a bit surprising that
--telemetry yes
doesn't mean accept telemetry, but rather yes to be prompted.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--telemetry yes
would mean consenting to telemetry, whileno
would mean not consenting. Both of those skip the prompt. I think my wording there might've been unclear.I've used the yes/no options because they mirror what's given to the user by the prompt when they run the
kedro
command for the first time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe here and for example_pipeline, we should leave it as it is: with yes/no option. Because if you want to choose
--telemetry=yes
,--skip-telemetry
will not provide you that option.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lrcouto @DimedS Thanks for clarifying!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with
--telemetry yes/no
👍🏽