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

refactor(ci): supporting git cliff for chosen projects #431

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

nimrod-starkware
Copy link
Contributor

@nimrod-starkware nimrod-starkware commented Aug 13, 2024

This change is Reviewable

@nimrod-starkware nimrod-starkware marked this pull request as ready for review August 13, 2024 13:52
Copy link
Contributor Author

nimrod-starkware commented Aug 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @nimrod-starkware and the rest of your teammates on Graphite Graphite

Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)


scripts/generate_changelog.py line 40 at r1 (raw file):

    ],
    "committer": ["committer", "committer_cli", "starknet_committer"],
    "starknet_api": ["starknet_api"],

Is this mapping correct?
I don't know where to put starknet_sierra_compile, task_executor, tests-integration, starknet_client

Code quote:

    "blockifier": ["blockifier", "native_blockifier"],
    "mempool": [
        "gateway",
        "mempool",
        "mempool_infra",
        "mempool_node",
        "mempool_test_utils",
        "mempool_types",
    ],
    "papyrus": [
        "papyrus_base_layer",
        "papyrus_common",
        "papyrus_config",
        "papyrus_execution",
        "papyrus_load_test",
        "papyrus_monitoring_gateway",
        "papyrus_network",
        "papyrus_node",
        "papyrus_p2p_sync",
        "papyrus_proc_macros",
        "papyrus_protobuf",
        "papyrus_rpc",
        "papyrus_storage",
        "papyrus_sync",
        "papyrus_test_utils",
        "sequencing",
    ],
    "committer": ["committer", "committer_cli", "starknet_committer"],
    "starknet_api": ["starknet_api"],

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)

a discussion (no related file):
did you manually test this?



scripts/generate_changelog.py line 3 at r1 (raw file):

#!/bin/env python3
import argparse
import itertools

do these need to be listed in requirements.txt?

Code quote:

import argparse
import itertools

scripts/generate_changelog.py line 5 at r1 (raw file):

import itertools

from merge_branches import run_command

run_command should probably be in it's own utils module

Code quote:

from merge_branches import run_command

scripts/generate_changelog.py line 39 at r1 (raw file):

        "sequencing",
    ],
    "committer": ["committer", "committer_cli", "starknet_committer"],

Suggestion:

["committer", "committer_cli", "starknet_committer", "starknet_patricia"]

scripts/generate_changelog.py line 40 at r1 (raw file):

Previously, nimrod-starkware wrote…

Is this mapping correct?
I don't know where to put starknet_sierra_compile, task_executor, tests-integration, starknet_client

neither do I. ask Dafna/DanB/ItayT about these crates?


scripts/generate_changelog.py line 48 at r1 (raw file):

def install_git_cliff(version: str):

calling this doesn't always install anything

Suggestion:

prepare_git_cliff

scripts/generate_changelog.py line 60 at r1 (raw file):

    if project_name not in PROJECT_NAMES:
        print(f"Invalid project name was given, given: {project_name} is not in {PROJECT_NAMES}")
        exit(1)

assert. the raised exception gives a traceback, which is useful

Suggestion:

    assert project_name in PROJECT_NAMES, f"Invalid project name was given, given: {project_name} is not in {PROJECT_NAMES}"

scripts/generate_changelog.py line 62 at r1 (raw file):

        exit(1)
    crates = PROJECT_TO_CRATES[project_name]
    include_paths = "".join((f"--include-path crates/{crate}/ " for crate in crates))

Suggestion:

include_paths = " ".join((f"--include-path crates/{crate}/" for crate in crates))

scripts/generate_changelog.py line 66 at r1 (raw file):

        f'git-cliff {start_tag}..{end_tag} -o changelog_{start_tag}_{end_tag}.md --ignore-tags ".*-dev.[0-9]+" '
        + include_paths
    )

Suggestion:

    return (
        f'git-cliff {start_tag}..{end_tag} -o changelog_{start_tag}_{end_tag}.md --ignore-tags ".*-dev.[0-9]+" '
        f'{include_paths}'
    )

scripts/generate_changelog.py line 74 at r1 (raw file):

        "--start", type=str, help="The commit that changelog's history starts from."
    )
    parser.add_argument("--end", type=str, help="The commit that changelog's history ends at.")

Suggestion:

    parser.add_argument(
        "--start", type=str, help="The commit/tag that changelog's history starts from."
    )
    parser.add_argument("--end", type=str, help="The commit/tag that changelog's history ends at.")

scripts/generate_changelog.py line 76 at r1 (raw file):

    parser.add_argument("--end", type=str, help="The commit that changelog's history ends at.")
    parser.add_argument(
        "--project", type=str, help="The project that the changelog is generated for."

use choices (I think this is the syntax)

Suggestion:

choices=PROJECT_TO_CRATES.keys()

Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 11 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @Itay-Tsabary-Starkware, and @TzahiTaub)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

did you manually test this?

yes, it works.
I didn't test the installation of git-cliff (was already installed), will do it now



scripts/generate_changelog.py line 3 at r1 (raw file):

Previously, dorimedini-starkware wrote…

do these need to be listed in requirements.txt?

I guess they do, no?


scripts/generate_changelog.py line 5 at r1 (raw file):

Previously, dorimedini-starkware wrote…

run_command should probably be in it's own utils module

Do you mean to create utils.py and move it there?


scripts/generate_changelog.py line 39 at r1 (raw file):

        "sequencing",
    ],
    "committer": ["committer", "committer_cli", "starknet_committer"],

Done


scripts/generate_changelog.py line 40 at r1 (raw file):

Previously, dorimedini-starkware wrote…

neither do I. ask Dafna/DanB/ItayT about these crates?

@dafnamatsry @dan-starkware @Itay-Tsabary-Starkware ?


scripts/generate_changelog.py line 48 at r1 (raw file):

Previously, dorimedini-starkware wrote…

calling this doesn't always install anything

Done.


scripts/generate_changelog.py line 60 at r1 (raw file):

Previously, dorimedini-starkware wrote…

assert. the raised exception gives a traceback, which is useful

I removed this check because the usage of choices


scripts/generate_changelog.py line 62 at r1 (raw file):

        exit(1)
    crates = PROJECT_TO_CRATES[project_name]
    include_paths = "".join((f"--include-path crates/{crate}/ " for crate in crates))

Done.


scripts/generate_changelog.py line 66 at r1 (raw file):

        f'git-cliff {start_tag}..{end_tag} -o changelog_{start_tag}_{end_tag}.md --ignore-tags ".*-dev.[0-9]+" '
        + include_paths
    )

Done.


scripts/generate_changelog.py line 74 at r1 (raw file):

        "--start", type=str, help="The commit that changelog's history starts from."
    )
    parser.add_argument("--end", type=str, help="The commit that changelog's history ends at.")

Done.


scripts/generate_changelog.py line 76 at r1 (raw file):

Previously, dorimedini-starkware wrote…

use choices (I think this is the syntax)

Done.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alon-dotan-starkware, @dafnamatsry, @dan-starkware, @Itay-Tsabary-Starkware, @nimrod-starkware, and @TzahiTaub)

a discussion (no related file):

Previously, nimrod-starkware wrote…

yes, it works.
I didn't test the installation of git-cliff (was already installed), will do it now

this part was tested in the previous .sh version, it's ok



scripts/generate_changelog.py line 3 at r1 (raw file):

Previously, nimrod-starkware wrote…

I guess they do, no?

not sure.
merge_paths_test imports from merge_branches, which already imports argparse for example,
but the CI phase that runs merge_paths_test only installs scripts/requirements.txt, which only contains GitPython as a dependency.
@alon-dotan-starkware when should we and when should we not add things to scripts/requirements.txt?


scripts/generate_changelog.py line 5 at r1 (raw file):

Previously, nimrod-starkware wrote…

Do you mean to create utils.py and move it there?

yes

@nimrod-starkware nimrod-starkware force-pushed the nimrod/git-cliff_script branch from 4862b45 to 600f10e Compare August 14, 2024 06:22
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @alon-dotan-starkware, @dafnamatsry, @dan-starkware, @dorimedini-starkware, @Itay-Tsabary-Starkware, and @TzahiTaub)


scripts/generate_changelog.py line 5 at r1 (raw file):

Previously, dorimedini-starkware wrote…

yes

Done.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @dafnamatsry, @dan-starkware, @Itay-Tsabary-Starkware, and @TzahiTaub)

Copy link
Collaborator

@dafnamatsry dafnamatsry left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @dan-starkware, @Itay-Tsabary-Starkware, and @TzahiTaub)


scripts/generate_changelog.py line 40 at r1 (raw file):

Previously, nimrod-starkware wrote…

@dafnamatsry @dan-starkware @Itay-Tsabary-Starkware ?

Not sure the division by the original repos make sense...
starknet_sierra_compile, task_executor, tests-integration originally came from the mempool repo.
But the first 2 are general utilities, not specific to the mempool.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @TzahiTaub)


scripts/generate_changelog.py line 40 at r1 (raw file):

Previously, dafnamatsry wrote…

Not sure the division by the original repos make sense...
starknet_sierra_compile, task_executor, tests-integration originally came from the mempool repo.
But the first 2 are general utilities, not specific to the mempool.

OK, since only the blockifier really needs release notes ATM,
and in the future hopefully the entire sequencer will have release notes,
let's have two options: blockifier, or everything.

@nimrod-starkware nimrod-starkware force-pushed the nimrod/git-cliff_script branch from 600f10e to a4e1a65 Compare August 15, 2024 06:45
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @dorimedini-starkware, and @TzahiTaub)


scripts/generate_changelog.py line 3 at r1 (raw file):

Previously, dorimedini-starkware wrote…

not sure.
merge_paths_test imports from merge_branches, which already imports argparse for example,
but the CI phase that runs merge_paths_test only installs scripts/requirements.txt, which only contains GitPython as a dependency.
@alon-dotan-starkware when should we and when should we not add things to scripts/requirements.txt?

This script is not part of the CI flow. Maybe there's no need to add it to requirements.txt then? It can be generated locally


scripts/generate_changelog.py line 9 at r4 (raw file):

# scripts/generate_changelog.py --start <FROM_TAG> --end <TO_TAG> --project <PROJECT_NAME>
GIT_CLIFF_VERSION = "2.4.0"
PROJECT_TO_PATHS = {"blockifier": ["crates/blockifier/"], "all": []}

@dorimedini-starkware IIRC you said that native_blockifiershould not be included, right?

Code quote:

["crates/blockifier/"]

Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @dorimedini-starkware, and @TzahiTaub)

a discussion (no related file):

Previously, dorimedini-starkware wrote…

this part was tested in the previous .sh version, it's ok

@dorimedini-starkware Other than that, do you want to enforce that the scope in commitlint is not empty?


Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

a discussion (no related file):

Previously, nimrod-starkware wrote…

@dorimedini-starkware Other than that, do you want to enforce that the scope in commitlint is not empty?

no need now IMO



scripts/generate_changelog.py line 3 at r1 (raw file):

Previously, nimrod-starkware wrote…

This script is not part of the CI flow. Maybe there's no need to add it to requirements.txt then? It can be generated locally

ok, if there's nothing to test then no need to add anything to requirements


scripts/generate_changelog.py line 9 at r4 (raw file):

Previously, nimrod-starkware wrote…

@dorimedini-starkware IIRC you said that native_blockifiershould not be included, right?

right. native_blockifier depends on blockifier, but not the other way around; and the native blockifier crate is (and will remain) unpublished

@nimrod-starkware nimrod-starkware merged commit 9956744 into main Aug 15, 2024
10 checks passed
@nimrod-starkware nimrod-starkware deleted the nimrod/git-cliff_script branch August 15, 2024 08:33
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants