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

Final clean up for kedro pipeline create and kedro catalog create docs #2918

Closed
Tracked by #2919
noklam opened this issue Aug 11, 2023 · 6 comments · Fixed by #2945
Closed
Tracked by #2919

Final clean up for kedro pipeline create and kedro catalog create docs #2918

noklam opened this issue Aug 11, 2023 · 6 comments · Fixed by #2945
Assignees
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@noklam
Copy link
Contributor

noklam commented Aug 11, 2023

Description

Steps to reprdouce:

kedro new -s spaceflights --checkout main 
kedro pipeline create new

Inconsistency 1 - kedro pipeline create add README.md while starter doesn't have it

image

Inconsistency 2 - CLI docstring seems to be outdated.

https://github.com/kedro-org/kedro/blob/6888001c6019059ae717c99fbe26a06f67d73ca2/kedro/framework/cli/catalog.py#L131C1-L141C8

Inconsistency 3 - broken doc

It generates a link that doesn't exist - https://docs.kedro.org/en/0.18.12/kedro_project_setup/configuration.html#parameters

See this:

# Link: https://docs.kedro.org/en/{{ cookiecutter.kedro_version }}/kedro_project_setup/configuration.html#parameters

Also, any idea it is in a folder called config but not conf?

Outdated Doc 4

See #2888 (comment)

Context

Possible Implementation

Possible Alternatives

@noklam noklam added the Issue: Feature Request New feature or improvement to existing feature label Aug 11, 2023
@noklam noklam changed the title Final clean up for kedro pipeline create and starters to ensure consistency Final clean up for kedro pipeline create and starters Aug 11, 2023
@amandakys
Copy link

just to confirm - this kedro pipeline create doesn't create the catalog_{pipeline}.yml file right?
I think unless you think there's a good reason to add the readme.md we can just remove it?
I think there's also another ticket tracking the update of the docs after this work.

@DimedS
Copy link
Contributor

DimedS commented Aug 11, 2023

Good remarks. Perhaps we should enhance the README.md in the starters pipelines with a good examples.
It seems the link should be updated to https://docs.kedro.org/en/0.18.12/configuration/parameters.html.
Also this would be a good place to add e2e test for 'kedro pipeline delete' that we previously discussed.

@noklam
Copy link
Contributor Author

noklam commented Aug 11, 2023

just to confirm - this kedro pipeline create doesn't create the catalog_{pipeline}.yml file right? I think unless you think there's a good reason to add the readme.md we can just remove it? I think there's also another ticket tracking the update of the docs after this work.

@amandakys - it doesn't create catalog_pipeline.yml, I think the current behavior is correct and consistent with the outcome of technical design. catalog_pipeline.yml only created by kedro catalog create.

On the other hand, I am in favor of deleting it as it doesn't add much value. Some users, especially for the larger project they do document pipelines separately with README.md. This should be quite easy to do themselves and I don't think it's necessary for us to add it in kedro pipeline create.

@merelcht
Copy link
Member

Inconsistency 1: discussed in backlog grooming all good to remove the pipeline level README.md.
Inconsistency 2: Change to:

The catalog configuration will be saved to
    `<conf_source>/<env>/catalog_<pipeline_name>.yml` file.

Inconsistency 3: link should be changed to # Link: https://docs.kedro.org/en/{{ cookiecutter.kedro_version }}/configuration.html#parameters

@noklam noklam changed the title Final clean up for kedro pipeline create and starters Final clean up for kedro pipeline create Aug 14, 2023
@noklam noklam changed the title Final clean up for kedro pipeline create Final clean up for kedro pipeline create and kedro catalog create docs Aug 17, 2023
@stichbury
Copy link
Contributor

Following some discussion in #2888 which updates Data Catalog docs, we agreed that we need to update the text that mentions this command in the two places it appears (data catalog docs and CLI reference). Please consider those changes part of the definition of done for this issue.

@noklam noklam self-assigned this Aug 17, 2023
@noklam
Copy link
Contributor Author

noklam commented Aug 17, 2023

@stichbury I actually just look into the PR again. It more likely we should fix it in #2888. It's likely due to when you start this docs change, the PR hasn't been merged so you have a older copy.

Because your change actually deleted the whole data_catalog.md, so it doesn't have any conflict on your PR. This PR is small and likely to get merged within a day or two.

I

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants