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 an option for kedro new to skip telemetry #3701

Merged
merged 29 commits into from
Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
90fcd26
First draft for telemetry consent flag on kedro new
lrcouto Mar 11, 2024
8d0d13b
Add functioning --telemetry option to kedro new
lrcouto Mar 12, 2024
a8b0036
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 12, 2024
482d093
Update tests to acknowledge new flag
lrcouto Mar 12, 2024
6504100
Add tests for kedro new --telemetry flag
lrcouto Mar 13, 2024
09f174b
Add changes to documentation and release notes
lrcouto Mar 13, 2024
8917e85
Minor change to docs
lrcouto Mar 13, 2024
2b3b046
Lint
lrcouto Mar 13, 2024
583e5e7
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 13, 2024
f8254d0
Remove outdated comment and correct type hint
lrcouto Mar 13, 2024
fb5a876
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 13, 2024
c2c3906
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 14, 2024
650f37c
Update docs/source/get_started/new_project.md
lrcouto Mar 14, 2024
19ac1e7
Lint
lrcouto Mar 14, 2024
ecc3780
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 15, 2024
329b05f
Minor change on release note
lrcouto Mar 16, 2024
31089ee
Merge branch 'add-option-skip-telemetry' of github.com:kedro-org/kedr…
lrcouto Mar 16, 2024
39b9474
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 18, 2024
8733b2a
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 18, 2024
6c4ce42
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 18, 2024
c11f197
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 20, 2024
b34539c
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 26, 2024
3ee92fa
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 26, 2024
ef827ef
Merge branch 'main' into add-option-skip-telemetry
lrcouto Mar 27, 2024
575929d
Merge branch 'main' into add-option-skip-telemetry
lrcouto Apr 2, 2024
603c894
Merge branch 'main' into add-option-skip-telemetry
lrcouto Apr 3, 2024
f9bcd70
Merge branch 'main' into add-option-skip-telemetry
lrcouto Apr 3, 2024
29184db
Merge branch 'main' into add-option-skip-telemetry
lrcouto Apr 3, 2024
51265e5
Merge branch 'main' into add-option-skip-telemetry
lrcouto Apr 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Major features and improvements
* Kedro commands now work from any subdirectory within a Kedro project.
* Kedro CLI now provides a better error message when project commands are run outside of a project i.e. `kedro run`
* Adds the `--telemetry` flag to `kedro new`, allowing the user to register consent to have user analytics collected as the project is created.

## Bug fixes and other changes
* Updated `kedro pipeline create` and `kedro pipeline delete` to read the base environment from the project settings.
Expand Down
7 changes: 6 additions & 1 deletion docs/source/get_started/new_project.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Select the tools by number, or `all` or follow the default to add `none`.

### Project examples

Finally, the CLI offers the option to include starter example code in the project:
The CLI offers the option to include starter example code in the project:

```text
Would you like to include an example pipeline? :
Expand Down Expand Up @@ -112,6 +112,11 @@ You can also enter this in a single line as follows:
kedro new --name=testproject --tools=lint,docs,pyspark --example=n
```

### Telemetry consent

The `--telemetry` flag offers the option to register consent to have user analytics collected in the moment of the creation of the project. This option bypasses the prompt to collect analytics that would otherwise appear on the moment the `kedro` command is invoked for the first time inside the project. In case the `--telemetry` flag is not used, the user will be prompted to accept or reject analytics collection as usual.

When creating your new Kedro project, use the values `yes` or `no` to register consent to have user analytics collected for this specific project.
Copy link
Contributor

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.

Copy link
Contributor Author

@lrcouto lrcouto Mar 13, 2024

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, while no 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.

Copy link
Contributor

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.

Copy link
Contributor

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!

Copy link
Member

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 👍🏽

lrcouto marked this conversation as resolved.
Show resolved Hide resolved

## Run the new project

Expand Down
26 changes: 23 additions & 3 deletions kedro/framework/cli/starters.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, I will let him know.

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

It must contain only y, n, YES, or NO (case insensitive).



@define(order=True)
class KedroStarterSpec:
Expand Down Expand Up @@ -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."""
Expand All @@ -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()
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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."
Expand Down
116 changes: 116 additions & 0 deletions tests/framework/cli/test_starters.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

test_starters is becoming so long 🥲 Might be time to prioritise: #3594

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
2 changes: 2 additions & 0 deletions tests/tools/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ def test_get_cli_structure_depth(self, mocker, fake_metadata):
"-t",
"--example",
"-e",
"--telemetry",
"-tc",
"--starter",
"-s",
"--name",
Expand Down