diff --git a/.github/workflows/bot_pr_approval.yaml b/.github/workflows/bot_pr_approval.yaml new file mode 100644 index 00000000..f284fd79 --- /dev/null +++ b/.github/workflows/bot_pr_approval.yaml @@ -0,0 +1,10 @@ +name: Provide approval for bot PRs + +on: + pull_request: + +jobs: + bot_pr_approval: + uses: canonical/operator-workflows/.github/workflows/bot_pr_approval.yaml@main + secrets: inherit + diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b84f5e9..87ba2e92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,37 @@ # Changelog -## [Unreleased] +## Unreleased + +## [v0.9.0] - 2024-04-04 + +### Added + +- Added a formatted representation of the UpdatePageAction dataclass for more + human-readable output. +- To address a race condition between the conflict check and when content is + updated on discourse, the action now checks that the content hasn't changed + since the conflict check was completed, before pushing updates to discourse. +- If metadata.yaml does not exist, read the name and doc information from the + charmcraft.yaml file. +- New input environment variable INPUT_CHARM_DIR. metadata.yaml or charmcraft.yaml + will be read from this directory instead of the base one and the documentation + will also be searched under this directory. + +## [v0.8.2] - 2024-02-16 + +### Fixed + +- Removed soft-conflict ignore button. `discourse-ahead-ok` tag, which was used + to indicate changes between discourse and git should no longer be used. Has been + removed, with tests updated to reflect this. + +## [v0.8.1] - 2024-01-18 + +### Fixed + +- Migration error where discourse is in-line with the default branch but the + base content tag is behind. This no longer attempts migration and now also + moves the tag to the latest commit on the default branch. ## [v0.8.0] - 2023-11-30 diff --git a/Dockerfile b/Dockerfile index 411f2052..eb678b66 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. -FROM python:3.11-slim +FROM python:3.12-slim RUN apt-get update && \ apt-get install -y --no-install-recommends git=1:2.39.* && \ diff --git a/README.md b/README.md index 29b4e82f..2d43b078 100644 --- a/README.md +++ b/README.md @@ -210,11 +210,6 @@ that have not been merged into `main` and a PR proposes changes to `docs/architecture.md` could make changes to the documentation that mean that the changes to `docs/getting-started.md` are no longer accurate. -If, after checking the community contributions on discourse, you determine that -there are no logical conflicts, the `discourse-gatekeeper/discourse-ahead-ok` tag -can be applied to the latest commit in the PR which will allow the action to -proceed assuming there are no page-by-page conflicts. - ## Contents Index The `docs/index.md` file may contain a `# contents` section which is used to diff --git a/action.yaml b/action.yaml index 032e59b6..cd6477d3 100644 --- a/action.yaml +++ b/action.yaml @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. name: Upload Charm Docs description: Upload charm documentation to charmhub @@ -55,6 +55,14 @@ inputs: variable GITHUB_SHA. required: false type: string + charm_dir: + description: | + Relative name of the directory where the metadata.yaml or charmcraft.yaml files are located + if they are not in the root directory of the repository. The docs directory is also located + under this directory. + default: '' + required: false + type: string outputs: index_url: description: | diff --git a/dev-requirements.txt b/dev-requirements.txt index c4cd0493..2b669c6b 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -2,5 +2,5 @@ ops pytest-operator factory_boy>=3,<4 pytest-asyncio>=0.21,<0.22 -pytest>=7,<8 +pytest>=8,<9 juju==3.1.2.0 diff --git a/generate-src-docs.sh b/generate-src-docs.sh index 43623058..e61c1e3d 100644 --- a/generate-src-docs.sh +++ b/generate-src-docs.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. rm -rf src-docs diff --git a/main.py b/main.py index 002609c5..7e8088a8 100755 --- a/main.py +++ b/main.py @@ -1,6 +1,6 @@ #!/usr/bin/env python -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Main execution for the action.""" @@ -45,6 +45,7 @@ def _parse_env_vars() -> types_.UserInputs: github_access_token = os.getenv("INPUT_GITHUB_TOKEN") base_branch = os.getenv("INPUT_BASE_BRANCH", DEFAULT_BRANCH) commit_sha = os.getenv("INPUT_COMMIT_SHA") + charm_dir = os.getenv("INPUT_CHARM_DIR", "") event_path = os.getenv("GITHUB_EVENT_PATH") if not event_path: @@ -79,6 +80,7 @@ def _parse_env_vars() -> types_.UserInputs: github_access_token=github_access_token, commit_sha=commit_sha, base_branch=base_branch, + charm_dir=charm_dir, ) diff --git a/pyproject.toml b/pyproject.toml index 10da48f6..9729678f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. [tool.bandit] diff --git a/requirements.txt b/requirements.txt index ebc0900f..28f28371 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,6 @@ GitPython>=3.1,<3.2 pydiscourse>=1.6,<1.7 -PyGithub>=2.1,<2.2 +PyGithub>=2.2,<2.3 PyYAML>=6.0,<6.1 requests>=2.31,<2.32 -more-itertools>=10.1,<10.2 +more-itertools>=10.2,<10.3 diff --git a/src-docs/__init__.py.md b/src-docs/__init__.py.md index f5bf1289..1c2b6a36 100644 --- a/src-docs/__init__.py.md +++ b/src-docs/__init__.py.md @@ -9,7 +9,6 @@ Library for uploading docs to charmhub. --------------- - **DRY_RUN_NAVLINK_LINK** - **FAIL_NAVLINK_LINK** -- **DOCUMENTATION_FOLDER_NAME** - **DOCUMENTATION_TAG** - **DEFAULT_BRANCH_NAME** - **GETTING_STARTED** @@ -51,7 +50,7 @@ Upload the documentation to charmhub. --- - + ## function `run_migrate` @@ -76,7 +75,7 @@ Migrate existing docs from charmhub to local repository. --- - + ## function `pre_flight_checks` diff --git a/src-docs/action.py.md b/src-docs/action.py.md index 7c5bb01d..6444b189 100644 --- a/src-docs/action.py.md +++ b/src-docs/action.py.md @@ -15,7 +15,7 @@ Module for taking the required actions to match the server state with the local --- - + ## function `run_all` diff --git a/src-docs/check.py.md b/src-docs/check.py.md index 45fcfc4a..e1b3babf 100644 --- a/src-docs/check.py.md +++ b/src-docs/check.py.md @@ -11,7 +11,7 @@ Module for running checks. --- - + ## function `get_path_with_diffs` @@ -37,15 +37,13 @@ Generate the paths that have either local or server content changes. --- - + ## function `conflicts` ```python conflicts( - actions: Iterable[CreateGroupAction | CreatePageAction | CreateExternalRefAction | NoopGroupAction | NoopPageAction | NoopExternalRefAction | UpdateGroupAction | UpdatePageAction | UpdateExternalRefAction | DeleteGroupAction | DeletePageAction | DeleteExternalRefAction], - repository: Client, - user_inputs: UserInputs + actions: Iterable[CreateGroupAction | CreatePageAction | CreateExternalRefAction | NoopGroupAction | NoopPageAction | NoopExternalRefAction | UpdateGroupAction | UpdatePageAction | UpdateExternalRefAction | DeleteGroupAction | DeletePageAction | DeleteExternalRefAction] ) → Iterator[Problem] ``` @@ -60,8 +58,6 @@ The second type of conflict is a logical conflict which arises out of that there **Args:** - `actions`: The actions to check. - - `repository`: Client for repository interactions. - - `user_inputs`: Configuration from the user. @@ -71,7 +67,7 @@ The second type of conflict is a logical conflict which arises out of that there --- - + ## function `external_refs` diff --git a/src-docs/constants.py.md b/src-docs/constants.py.md index ddc0b15a..3c950b6b 100644 --- a/src-docs/constants.py.md +++ b/src-docs/constants.py.md @@ -11,7 +11,6 @@ The use of this module should be limited to cases where the constant is not bett --------------- - **DEFAULT_BRANCH** - **DOCUMENTATION_TAG** -- **DISCOURSE_AHEAD_TAG** - **DOCUMENTATION_FOLDER_NAME** - **DOC_FILE_EXTENSION** - **DOCUMENTATION_INDEX_FILENAME** diff --git a/src-docs/docs_directory.py.md b/src-docs/docs_directory.py.md index 0d9dfa94..77ca760c 100644 --- a/src-docs/docs_directory.py.md +++ b/src-docs/docs_directory.py.md @@ -8,7 +8,6 @@ Class for reading the docs directory. **Global Variables** --------------- - **DOC_FILE_EXTENSION** -- **DOCUMENTATION_FOLDER_NAME** --- @@ -67,7 +66,7 @@ Algorithm: 1. Get a list of all sub directories and .md files in the docs fold ## function `has_docs_directory` ```python -has_docs_directory(base_path: Path) → bool +has_docs_directory(docs_path: Path) → bool ``` Return existence of docs directory from base path. @@ -76,7 +75,7 @@ Return existence of docs directory from base path. **Args:** - - `base_path`: Base path of the repository to search the docs directory from + - `docs_path`: Docs path of the repository where docs are diff --git a/src-docs/download.py.md b/src-docs/download.py.md index 54d40629..057eb2f7 100644 --- a/src-docs/download.py.md +++ b/src-docs/download.py.md @@ -5,13 +5,10 @@ # module `download.py` Library for downloading docs folder from charmhub. -**Global Variables** ---------------- -- **DOCUMENTATION_FOLDER_NAME** --- - + ## function `recreate_docs` diff --git a/src-docs/index.py.md b/src-docs/index.py.md index cd6f2681..96ddb6fd 100644 --- a/src-docs/index.py.md +++ b/src-docs/index.py.md @@ -8,7 +8,6 @@ Execute the uploading of documentation. **Global Variables** --------------- - **DOC_FILE_EXTENSION** -- **DOCUMENTATION_FOLDER_NAME** - **DOCUMENTATION_INDEX_FILENAME** - **NAVIGATION_HEADING** - **CONTENTS_HEADER** @@ -16,12 +15,12 @@ Execute the uploading of documentation. --- - + ## function `get` ```python -get(metadata: Metadata, base_path: Path, server_client: Discourse) → Index +get(metadata: Metadata, docs_path: Path, server_client: Discourse) → Index ``` Retrieve the local and server index information. @@ -31,7 +30,7 @@ Retrieve the local and server index information. **Args:** - `metadata`: Information about the charm. - - `base_path`: The base path to look for the metadata file in. + - `docs_path`: The base path to look for the documentation. - `server_client`: A client to the documentation server. @@ -48,7 +47,7 @@ Retrieve the local and server index information. --- - + ## function `contents_from_page` @@ -72,7 +71,7 @@ Get index file contents from server page. --- - + ## function `get_content_for_server` @@ -96,7 +95,7 @@ Get the contents from the index file that should be passed to the server. --- - + ## function `classify_item_reference` @@ -124,7 +123,7 @@ Classify the type of a reference. --- - + ## function `get_contents` diff --git a/src-docs/metadata.py.md b/src-docs/metadata.py.md index 875d603c..4bf6d711 100644 --- a/src-docs/metadata.py.md +++ b/src-docs/metadata.py.md @@ -7,13 +7,17 @@ Module for parsing metadata.yaml file. **Global Variables** --------------- +- **CHARMCRAFT_FILENAME** +- **CHARMCRAFT_NAME_KEY** +- **CHARMCRAFT_LINKS_KEY** +- **CHARMCRAFT_LINKS_DOCS_KEY** - **METADATA_DOCS_KEY** - **METADATA_FILENAME** - **METADATA_NAME_KEY** --- - + ## function `get` @@ -23,11 +27,13 @@ get(path: Path) → Metadata Check for and read the metadata. +The charm metadata can be in the file metadata.yaml or in charmcraft.yaml. From charmcraft version 2.5, the information should be in charmcraft.yaml, and the user should only modify that file. This function does not consider the case in which the name is in one file and the doc link is in the other. + **Args:** - - `path`: The base path to look for the metadata file in. + - `path`: The base path to look for the metadata files. @@ -38,6 +44,6 @@ Check for and read the metadata. **Raises:** - - `InputError`: if the metadata file does not exists or is malformed. + - `InputError`: if the metadata file does not exist or is malformed. diff --git a/src-docs/repository.py.md b/src-docs/repository.py.md index 5d721721..92e31170 100644 --- a/src-docs/repository.py.md +++ b/src-docs/repository.py.md @@ -25,12 +25,16 @@ Module for handling interactions with git repository. --- - + ## function `create_repository_client` ```python -create_repository_client(access_token: str | None, base_path: Path) → Client +create_repository_client( + access_token: str | None, + base_path: Path, + charm_dir: str = '' +) → Client ``` Create a Github instance to handle communication with Github server. @@ -41,6 +45,7 @@ Create a Github instance to handle communication with Github server. - `access_token`: Access token that has permissions to open a pull request. - `base_path`: Path where local .git resides in. + - `charm_dir`: Relative directory where the charm files are located. @@ -59,14 +64,18 @@ Create a Github instance to handle communication with Github server. ## class `Client` Wrapper for git/git-server related functionalities. -Attrs: base_path: The root directory of the repository. metadata: Metadata object of the charm has_docs_directory: whether the repository has a docs directory current_branch: current git branch used in the repository current_commit: current commit checkout in the repository branches: list of all branches +Attrs: base_path: The root directory of the repository. base_charm_path: The directory of the repository where the charm is. docs_path: The directory of the repository where the documentation is. metadata: Metadata object of the charm has_docs_directory: whether the repository has a docs directory current_branch: current git branch used in the repository current_commit: current commit checkout in the repository branches: list of all branches - + ### function `__init__` ```python -__init__(repository: Repo, github_repository: Repository) → None +__init__( + repository: Repo, + github_repository: Repository, + charm_dir: str = '' +) → None ``` Construct. @@ -77,7 +86,19 @@ Construct. - `repository`: Client for interacting with local git repository. - `github_repository`: Client for interacting with remote github repository. + - `charm_dir`: Relative directory where charm files are located. + + +--- + +#### property base_charm_path +Return the Path of the charm in the repository. + + + +**Returns:** + Path of the repository. --- @@ -99,6 +120,17 @@ Return the current branch. --- +#### property docs_path + +Return the Path of the charm in the repository. + + + +**Returns:** + Path of the repository. + +--- + #### property has_docs_directory Return whether the repository has a docs directory. @@ -113,7 +145,7 @@ Return the Metadata object of the charm. --- - + ### function `create_branch` @@ -147,7 +179,7 @@ repository.create_branch(branch_name).switch(branch_name) --- - + ### function `create_pull_request` @@ -176,7 +208,7 @@ Create pull request for changes in given repository path. --- - + ### function `get_file_content_from_tag` @@ -208,7 +240,7 @@ Get the content of a file for a specific tag. --- - + ### function `get_pull_request` @@ -237,12 +269,12 @@ Return open pull request matching the provided branch name. --- - + ### function `get_summary` ```python -get_summary(directory: str | None = 'docs') → DiffSummary +get_summary(directory: str | Path | None) → DiffSummary ``` Return a summary of the differences against the most recent commit. @@ -260,7 +292,7 @@ Return a summary of the differences against the most recent commit. --- - + ### function `is_commit_in_branch` @@ -290,7 +322,7 @@ Check if commit exists in a given branch. --- - + ### function `is_dirty` @@ -313,7 +345,7 @@ Check if repository path has any changes including new files. --- - + ### function `is_same_commit` @@ -337,7 +369,7 @@ Return whether tag and commit coincides. --- - + ### function `pull` @@ -355,7 +387,7 @@ Pull content from remote for the provided branch. --- - + ### function `switch` @@ -378,7 +410,7 @@ Switch branch for the repository. --- - + ### function `tag_commit` @@ -403,7 +435,7 @@ Tag a commit, if the tag already exists, it is deleted first. --- - + ### function `tag_exists` @@ -426,16 +458,16 @@ Check if a given tag exists. --- - + ### function `update_branch` ```python update_branch( commit_msg: str, + directory: str | Path | None, push: bool = True, - force: bool = False, - directory: str | None = 'docs' + force: bool = False ) → Client ``` @@ -463,7 +495,7 @@ Update branch with a new commit. --- - + ### function `update_pull_request` @@ -481,7 +513,7 @@ Update and push changes to the given branch. --- - + ### function `with_branch` diff --git a/src-docs/types_.py.md b/src-docs/types_.py.md index edf8871d..51d1051e 100644 --- a/src-docs/types_.py.md +++ b/src-docs/types_.py.md @@ -371,7 +371,7 @@ Whether the row is a group of pages. --- - + ### function `is_external` @@ -394,7 +394,7 @@ Whether the row is an external reference. --- - + ### function `to_markdown` @@ -461,7 +461,7 @@ Attrs: content_change: The change to the documentation content. ## class `UserInputs` Configurable user input values used to run discourse-gatekeeper. -Attrs: discourse: The configuration for interacting with discourse. dry_run: If enabled, only log the action that would be taken. Has no effect in migration mode. delete_pages: Whether to delete pages that are no longer needed. Has no effect in migration mode. github_access_token: A Personal Access Token(PAT) or access token with repository access. Required in migration mode. commit_sha: The SHA of the commit the action is running on. base_branch: The main branch against which the syncs act on +Attrs: discourse: The configuration for interacting with discourse. dry_run: If enabled, only log the action that would be taken. Has no effect in migration mode. delete_pages: Whether to delete pages that are no longer needed. Has no effect in migration mode. github_access_token: A Personal Access Token(PAT) or access token with repository access. Required in migration mode. commit_sha: The SHA of the commit the action is running on. base_branch: The main branch against which the syncs act on. charm_dir: Directory the charm is located in. diff --git a/src/__init__.py b/src/__init__.py index 282b81fd..30405066 100644 --- a/src/__init__.py +++ b/src/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Library for uploading docs to charmhub.""" @@ -12,7 +12,7 @@ from src import sort as sort_module from src.action import DRY_RUN_NAVLINK_LINK, FAIL_NAVLINK_LINK from src.clients import Clients -from src.constants import DOCUMENTATION_FOLDER_NAME, DOCUMENTATION_TAG +from src.constants import DOCUMENTATION_TAG from src.download import recreate_docs from src.exceptions import InputError, TaggingNotAllowedError from src.repository import DEFAULT_BRANCH_NAME @@ -50,7 +50,7 @@ def _get_reconcile_actions( Raises: InputError: if there are any problems with the contents index. """ - docs_path = clients.repository.base_path / DOCUMENTATION_FOLDER_NAME + docs_path = clients.repository.docs_path path_infos = docs_directory.read(docs_path=docs_path) index_contents = index_module.get_contents(index_file=index.local, docs_path=docs_path) @@ -105,7 +105,7 @@ def run_reconcile(clients: Clients, user_inputs: UserInputs) -> ReconcileOutputs index = index_module.get( metadata=clients.repository.metadata, - base_path=clients.repository.base_path, + docs_path=clients.repository.docs_path, server_client=clients.discourse, ) server_content = ( @@ -133,7 +133,7 @@ def run_reconcile(clients: Clients, user_inputs: UserInputs) -> ReconcileOutputs index_url=index.server.url if index.server else "", topics=( { - f"{clients.discourse.absolute_url(row.navlink.link)}": ActionResult.SKIP + clients.discourse.absolute_url(row.navlink.link): ActionResult.SKIP for row in table_rows if row.navlink.link } @@ -147,11 +147,7 @@ def run_reconcile(clients: Clients, user_inputs: UserInputs) -> ReconcileOutputs ) actions, check_actions = tee(actions, 2) - problems = tuple( - check.conflicts( - actions=check_actions, repository=clients.repository, user_inputs=user_inputs - ) - ) + problems = tuple(check.conflicts(actions=check_actions)) if problems: raise InputError( "One or more of the required actions could not be executed, see the log for details" @@ -221,11 +217,19 @@ def run_migrate(clients: Clients, user_inputs: UserInputs) -> MigrateOutputs | N # Check difference with main changes = recreate_docs(clients, DOCUMENTATION_TAG) + # Check whether there are still changes after switching to the base branch + if changes: + changes = clients.repository.is_dirty(user_inputs.base_branch) + + # Move the tag if there are no changes + if not changes: + with clients.repository.with_branch(user_inputs.base_branch) as repo: + main_hash = repo.current_commit + clients.repository.tag_commit(DOCUMENTATION_TAG, main_hash) + if not changes: logging.info( - "No community contribution found in commit %s. Discourse is inline with %s", - user_inputs.commit_sha, - DOCUMENTATION_TAG, + "No community contribution found, discourse is inline with %s", DOCUMENTATION_TAG ) # Given there are NO diffs compared to the base, if a PR is open, it should be closed if pull_request is not None: diff --git a/src/action.py b/src/action.py index 462987aa..1702da92 100644 --- a/src/action.py +++ b/src/action.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for taking the required actions to match the server state with the local state.""" @@ -192,6 +192,10 @@ def _update( discourse: A client to the documentation server. dry_run: If enabled, only log the action that would be taken. + Raises: + ActionError: if there have been changes to the content on the server + since the conflict check was done. + Returns: A report on the outcome of executing the action. """ @@ -212,15 +216,24 @@ def _update( case UpdateCase.CONTENT_CHANGE: action = typing.cast(types_.UpdatePageAction, action) try: + topic_url = typing.cast(str, action.navlink_change.new.link) content_change = typing.cast(types_.ContentChange, action.content_change) + + # Check that content has not changed since the conflict check was performed + current_server_content = discourse.retrieve_topic(url=topic_url) + if current_server_content != content_change.server: + raise exceptions.ActionError( + f"The content being updated at {topic_url} has changed since the conflict " + "check was done. Please resolve any conflicts and re-run the action." + ) + + # Apply the change merged_content = content.merge( base=typing.cast(str, content_change.base), theirs=content_change.server, ours=content_change.local, ) - discourse.update_topic( - url=typing.cast(str, action.navlink_change.new.link), content=merged_content - ) + discourse.update_topic(url=topic_url, content=merged_content) result = types_.ActionResult.SUCCESS reason = None except (exceptions.DiscourseError, exceptions.ContentError) as exc: diff --git a/src/check.py b/src/check.py index fd367d79..f5e1a208 100644 --- a/src/check.py +++ b/src/check.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for running checks.""" @@ -10,9 +10,8 @@ import requests -from src import constants, content +from src import content from src.constants import DOCUMENTATION_TAG -from src.repository import Client from src.types_ import ( AnyAction, IndexContentsListItem, @@ -20,7 +19,6 @@ UpdateExternalRefAction, UpdateGroupAction, UpdatePageAction, - UserInputs, ) @@ -158,9 +156,7 @@ def _update_action_problem(action: UpdateAction) -> Problem | None: return problem -def conflicts( - actions: Iterable[AnyAction], repository: Client, user_inputs: UserInputs -) -> Iterator[Problem]: +def conflicts(actions: Iterable[AnyAction]) -> Iterator[Problem]: """Check whether actions have any content conflicts. There are two types of conflicts. The first is where the local content is different to what is @@ -176,8 +172,6 @@ def conflicts( Args: actions: The actions to check. - repository: Client for repository interactions. - user_inputs: Configuration from the user. Yields: A problem for each action with a conflict @@ -196,12 +190,6 @@ def conflicts( if any_page_conflicts: return - commit_discourse_ahead_ok_tagged = repository.is_same_commit( - tag=constants.DISCOURSE_AHEAD_TAG, commit=user_inputs.commit_sha - ) - if commit_discourse_ahead_ok_tagged: - return - paths_with_diff = get_path_with_diffs(actions_logical_conflicts) if not paths_with_diff.base_local_diffs or not paths_with_diff.base_server_diffs: return @@ -210,9 +198,7 @@ def conflicts( path=paths_with_diff.base_local_diffs[0], description=( "detected unmerged community contributions, these need to be resolved " - "before proceeding. If the differences are not conflicting, please apply the " - f"{constants.DISCOURSE_AHEAD_TAG} tag to commit {user_inputs.commit_sha} to " - "proceed. Paths with potentially unmerged community contributions: " + "before proceeding. Paths with potentially unmerged community contributions: " f"{set(chain(paths_with_diff.base_local_diffs, paths_with_diff.base_server_diffs))}." ), ) diff --git a/src/clients.py b/src/clients.py index dec0ebe0..c9d732a3 100644 --- a/src/clients.py +++ b/src/clients.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for Client class.""" @@ -42,6 +42,8 @@ def get_clients(user_inputs: UserInputs, base_path: Path) -> Clients: api_key=user_inputs.discourse.api_key, ), repository=create_repository_client( - access_token=user_inputs.github_access_token, base_path=base_path + access_token=user_inputs.github_access_token, + base_path=base_path, + charm_dir=user_inputs.charm_dir, ), ) diff --git a/src/commit.py b/src/commit.py index 215fbeff..b2f1e9c0 100644 --- a/src/commit.py +++ b/src/commit.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for handling interactions with git commit.""" diff --git a/src/constants.py b/src/constants.py index bd283583..fa2cb5bb 100644 --- a/src/constants.py +++ b/src/constants.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Shared constants. @@ -8,7 +8,6 @@ """ DEFAULT_BRANCH = "main" DOCUMENTATION_TAG = "discourse-gatekeeper/base-content" -DISCOURSE_AHEAD_TAG = "discourse-gatekeeper/discourse-ahead-ok" DOCUMENTATION_FOLDER_NAME = "docs" DOC_FILE_EXTENSION = ".md" diff --git a/src/content.py b/src/content.py index cc14857a..9ffea9a5 100644 --- a/src/content.py +++ b/src/content.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for checking conflicts using 3-way merge and create content based on a 3 way merge.""" diff --git a/src/discourse.py b/src/discourse.py index 0b574be2..8af01af4 100644 --- a/src/discourse.py +++ b/src/discourse.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Interface for Discourse interactions.""" diff --git a/src/docs_directory.py b/src/docs_directory.py index 867e1228..5e9230be 100644 --- a/src/docs_directory.py +++ b/src/docs_directory.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Class for reading the docs directory.""" @@ -9,7 +9,7 @@ from pathlib import Path from src import types_ -from src.constants import DOC_FILE_EXTENSION, DOCUMENTATION_FOLDER_NAME +from src.constants import DOC_FILE_EXTENSION def _get_directories_files(docs_path: Path) -> list[Path]: @@ -152,13 +152,13 @@ def read(docs_path: Path) -> typing.Iterator[types_.PathInfo]: ) -def has_docs_directory(base_path: Path) -> bool: +def has_docs_directory(docs_path: Path) -> bool: """Return existence of docs directory from base path. Args: - base_path: Base path of the repository to search the docs directory from + docs_path: Docs path of the repository where docs are Returns: True if documentation folder exists, False otherwise """ - return (base_path / DOCUMENTATION_FOLDER_NAME).is_dir() + return docs_path.is_dir() diff --git a/src/download.py b/src/download.py index eaf7593a..f16d9809 100644 --- a/src/download.py +++ b/src/download.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Library for downloading docs folder from charmhub.""" @@ -6,7 +6,6 @@ import shutil from src.clients import Clients -from src.constants import DOCUMENTATION_FOLDER_NAME from src.index import contents_from_page from src.index import get as get_index from src.migration import run as migrate_contents @@ -19,10 +18,10 @@ def _download_from_discourse(clients: Clients) -> None: Args: clients: Clients object """ - base_path = clients.repository.base_path + docs_path = clients.repository.docs_path metadata = clients.repository.metadata - index = get_index(metadata=metadata, base_path=base_path, server_client=clients.discourse) + index = get_index(metadata=metadata, docs_path=docs_path, server_client=clients.discourse) server_content = ( index.server.content if index.server is not None and index.server.content else "" ) @@ -32,7 +31,7 @@ def _download_from_discourse(clients: Clients) -> None: table_rows=table_rows, index_content=index_content, discourse=clients.discourse, - docs_path=base_path / DOCUMENTATION_FOLDER_NAME, + docs_path=docs_path, ) @@ -49,7 +48,7 @@ def recreate_docs(clients: Clients, base: str) -> bool: clients.repository.switch(base) # Remove docs folder and recreate content from discourse - docs_path = clients.repository.base_path / DOCUMENTATION_FOLDER_NAME + docs_path = clients.repository.docs_path if docs_path.exists(): shutil.rmtree(docs_path) diff --git a/src/exceptions.py b/src/exceptions.py index caba84a7..a71fbafa 100644 --- a/src/exceptions.py +++ b/src/exceptions.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Exceptions for uploading docs to charmhub.""" diff --git a/src/index.py b/src/index.py index 3a4a29b1..46a3eedf 100644 --- a/src/index.py +++ b/src/index.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Execute the uploading of documentation.""" @@ -10,12 +10,7 @@ from enum import Enum, auto from pathlib import Path -from src.constants import ( - DOC_FILE_EXTENSION, - DOCUMENTATION_FOLDER_NAME, - DOCUMENTATION_INDEX_FILENAME, - NAVIGATION_HEADING, -) +from src.constants import DOC_FILE_EXTENSION, DOCUMENTATION_INDEX_FILENAME, NAVIGATION_HEADING from src.discourse import Discourse from src.exceptions import DiscourseError, InputError, ServerError from src.types_ import Index, IndexContentsListItem, IndexFile, Metadata, Page @@ -34,30 +29,30 @@ _HIDDEN_ITEM_PATTERN = re.compile(_HIDDEN_ITEM) -def _read_docs_index(base_path: Path) -> str | None: +def _read_docs_index(docs_path: Path) -> str | None: """Read the content of the index file. Args: - base_path: The starting path to look for the index content. + docs_path: The path to look for the index content. Returns: The content of the index file if it exists, otherwise return None. """ - if not (docs_directory := base_path / DOCUMENTATION_FOLDER_NAME).is_dir(): + if not docs_path.is_dir(): return None - if not (index_file := docs_directory / DOCUMENTATION_INDEX_FILENAME).is_file(): + if not (index_file := docs_path / DOCUMENTATION_INDEX_FILENAME).is_file(): return None return index_file.read_text() -def get(metadata: Metadata, base_path: Path, server_client: Discourse) -> Index: +def get(metadata: Metadata, docs_path: Path, server_client: Discourse) -> Index: """Retrieve the local and server index information. Args: metadata: Information about the charm. - base_path: The base path to look for the metadata file in. + docs_path: The base path to look for the documentation. server_client: A client to the documentation server. Returns: @@ -78,7 +73,7 @@ def get(metadata: Metadata, base_path: Path, server_client: Discourse) -> Index: server = None name_value = metadata.name - local_content = _read_docs_index(base_path=base_path) + local_content = _read_docs_index(docs_path=docs_path) local = IndexFile( title=f"{name_value.replace('-', ' ').title()} Documentation Overview", content=local_content, diff --git a/src/metadata.py b/src/metadata.py index 1d701ccd..ec4de83d 100644 --- a/src/metadata.py +++ b/src/metadata.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for parsing metadata.yaml file.""" @@ -10,6 +10,10 @@ from src import types_ from src.exceptions import InputError +CHARMCRAFT_FILENAME = "charmcraft.yaml" +CHARMCRAFT_NAME_KEY = "name" +CHARMCRAFT_LINKS_KEY = "links" +CHARMCRAFT_LINKS_DOCS_KEY = "documentation" METADATA_DOCS_KEY = "docs" METADATA_FILENAME = "metadata.yaml" METADATA_NAME_KEY = "name" @@ -18,20 +22,47 @@ def get(path: Path) -> types_.Metadata: """Check for and read the metadata. + The charm metadata can be in the file metadata.yaml or in charmcraft.yaml. + From charmcraft version 2.5, the information should be in charmcraft.yaml, + and the user should only modify that file. This function does not consider + the case in which the name is in one file and the doc link is in the other. + Args: - path: The base path to look for the metadata file in. + path: The base path to look for the metadata files. Returns: The contents of the metadata file. Raises: - InputError: if the metadata file does not exists or is malformed. + InputError: if the metadata file does not exist or is malformed. """ metadata_yaml = path / METADATA_FILENAME - if not metadata_yaml.is_file(): - raise InputError(f"Could not find {METADATA_FILENAME} file, looked in folder: {path}") + if metadata_yaml.is_file(): + return _parse_metadata_yaml(metadata_yaml) + + charmcraft_yaml = path / CHARMCRAFT_FILENAME + if charmcraft_yaml.is_file(): + return _parse_charmcraft_yaml(charmcraft_yaml) + + raise InputError( + f"Could not find {METADATA_FILENAME} or {CHARMCRAFT_FILENAME} files" + f", looked in folder: {path}" + ) + + +def _parse_metadata_yaml(metadata_yaml: Path) -> types_.Metadata: + """Parse metadata file. + + Args: + metadata_yaml: The file path the the metadata file. + + Returns: + The contents of the metadata file. + Raises: + InputError: if the metadata file does not exist or are malformed. + """ try: metadata = yaml.safe_load(metadata_yaml.read_text()) except yaml.error.YAMLError as exc: @@ -62,3 +93,55 @@ def get(path: Path) -> types_.Metadata: raise InputError(f"Invalid value for docs key: {docs}, expected a string value") return types_.Metadata(name=name, docs=docs) + + +def _parse_charmcraft_yaml(charmcraft_yaml: Path) -> types_.Metadata: + """Parse charmcraft file. + + Args: + charmcraft_yaml: The file path the the charmcraft file. + + Returns: + The contents of the charmcraft file. + + Raises: + InputError: if the charmcraft file does not exist or are malformed. + """ + try: + charmcraft = yaml.safe_load(charmcraft_yaml.read_text()) + except yaml.error.YAMLError as exc: + raise InputError( + f"Malformed {CHARMCRAFT_FILENAME} file, read file: {charmcraft_yaml}" + ) from exc + + if not charmcraft: + raise InputError(f"{CHARMCRAFT_FILENAME} file is empty, read file: {charmcraft_yaml}") + if not isinstance(charmcraft, dict): + raise InputError( + f"{CHARMCRAFT_FILENAME} file does not contain a mapping at the root, " + f"read file: {charmcraft_yaml}, content: {charmcraft!r}" + ) + + if CHARMCRAFT_NAME_KEY not in charmcraft: + raise InputError( + f"Could not find required key: {CHARMCRAFT_NAME_KEY}, " + f"read file: {charmcraft_yaml}, content: {charmcraft!r}" + ) + if not isinstance(name := charmcraft[CHARMCRAFT_NAME_KEY], str): + raise InputError(f"Invalid value for name key: {name}, expected a string value") + + docs = None + links = charmcraft.get(CHARMCRAFT_LINKS_KEY) + if links: + if not isinstance(links, dict): + raise InputError( + f"{CHARMCRAFT_FILENAME} invalid value for links {CHARMCRAFT_LINKS_KEY} key." + ) + + docs = links.get(CHARMCRAFT_LINKS_DOCS_KEY) + if not (isinstance(docs, str) or docs is None): + raise InputError( + f"Invalid value for documentation key: {docs}, expected a string value" + ) + + return types_.Metadata(name=name, docs=docs) diff --git a/src/migration.py b/src/migration.py index 2c1a9bdf..185d8629 100644 --- a/src/migration.py +++ b/src/migration.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for migrating remote documentation into local git repository.""" diff --git a/src/navigation_table.py b/src/navigation_table.py index 0cd9f72b..108254c4 100644 --- a/src/navigation_table.py +++ b/src/navigation_table.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for parsing and rendering a navigation table.""" diff --git a/src/reconcile.py b/src/reconcile.py index c02dea06..20e62258 100644 --- a/src/reconcile.py +++ b/src/reconcile.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for calculating required changes based on docs directory and navigation table.""" diff --git a/src/repository.py b/src/repository.py index 3a7a7403..96043b42 100644 --- a/src/repository.py +++ b/src/repository.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Module for handling interactions with git repository.""" @@ -168,6 +168,8 @@ class Client: # pylint: disable=too-many-public-methods Attrs: base_path: The root directory of the repository. + base_charm_path: The directory of the repository where the charm is. + docs_path: The directory of the repository where the documentation is. metadata: Metadata object of the charm has_docs_directory: whether the repository has a docs directory current_branch: current git branch used in the repository @@ -175,15 +177,19 @@ class Client: # pylint: disable=too-many-public-methods branches: list of all branches """ - def __init__(self, repository: Repo, github_repository: Repository) -> None: + def __init__( + self, repository: Repo, github_repository: Repository, charm_dir: str = "" + ) -> None: """Construct. Args: repository: Client for interacting with local git repository. github_repository: Client for interacting with remote github repository. + charm_dir: Relative directory where charm files are located. """ self._git_repo = repository self._github_repo = github_repository + self._charm_dir = charm_dir self._configure_git_user() @cached_property @@ -195,15 +201,33 @@ def base_path(self) -> Path: """ return Path(self._git_repo.working_tree_dir or self._git_repo.common_dir) + @property + def base_charm_path(self) -> Path: + """Return the Path of the charm in the repository. + + Returns: + Path of the repository. + """ + return self.base_path / self._charm_dir + + @property + def docs_path(self) -> Path: + """Return the Path of the charm in the repository. + + Returns: + Path of the repository. + """ + return self.base_charm_path / DOCUMENTATION_FOLDER_NAME + @property def metadata(self) -> Metadata: """Return the Metadata object of the charm.""" - return get_metadata(self.base_path) + return get_metadata(self.base_charm_path) @property def has_docs_directory(self) -> bool: """Return whether the repository has a docs directory.""" - return has_docs_directory(self.base_path) + return has_docs_directory(self.docs_path) @property def current_branch(self) -> str: @@ -248,7 +272,7 @@ def with_branch(self, branch_name: str) -> Iterator["Client"]: finally: self.switch(current_branch) - def get_summary(self, directory: str | None = DOCUMENTATION_FOLDER_NAME) -> DiffSummary: + def get_summary(self, directory: str | Path | None) -> DiffSummary: """Return a summary of the differences against the most recent commit. Args: @@ -258,7 +282,8 @@ def get_summary(self, directory: str | None = DOCUMENTATION_FOLDER_NAME) -> Diff Returns: DiffSummary object representing the summary of the differences. """ - self._git_repo.git.add(directory or ".") + directory = str(directory) if directory else "." + self._git_repo.git.add(directory) return DiffSummary.from_raw_diff( self._git_repo.index.diff(None) @@ -349,7 +374,7 @@ def _safe_pop_stash(self, branch_name: str) -> None: "Using stashed version.", branch_name, ) - self._git_repo.git.checkout("--theirs", DOCUMENTATION_FOLDER_NAME) + self._git_repo.git.checkout("--theirs", str(self.docs_path)) else: raise RepositoryClientError( f"Unexpected error when switching branch to {branch_name}. {exc=!r}" @@ -404,9 +429,9 @@ def _github_client_push( def update_branch( self, commit_msg: str, + directory: str | Path | None, push: bool = True, force: bool = False, - directory: str | None = DOCUMENTATION_FOLDER_NAME, ) -> "Client": """Update branch with a new commit. @@ -423,6 +448,7 @@ def update_branch( Returns: Repository client with the updated branch """ + directory = str(directory) if directory else "." push_args = ["-u"] if force: push_args.append("-f") @@ -433,7 +459,7 @@ def update_branch( if push: self._git_repo.git.push(*push_args) - self._git_repo.git.add("-A", directory or ".") + self._git_repo.git.add("-A", directory) self._git_repo.git.commit("-m", f"'{commit_msg}'") if push: try: @@ -535,9 +561,9 @@ def create_pull_request(self, base: str) -> PullRequest: with self.create_branch(DEFAULT_BRANCH_NAME, base).with_branch( DEFAULT_BRANCH_NAME ) as repo: - msg = str(repo.get_summary()) + msg = str(repo.get_summary(self.docs_path)) logging.info("Creating new branch with new commit: %s", msg) - repo.update_branch(msg, force=True) + repo.update_branch(msg, directory=self.docs_path, force=True) pull_request = _create_github_pull_request( self._github_repo, DEFAULT_BRANCH_NAME, base ) @@ -554,10 +580,10 @@ def update_pull_request(self, branch: str) -> None: with self.with_branch(branch) as repo: if repo.is_dirty(): repo.pull() - msg = str(repo.get_summary()) + msg = str(repo.get_summary(self.docs_path)) logging.info("Summary: %s", msg) logging.info("Updating PR with new commit: %s", msg) - repo.update_branch(msg) + repo.update_branch(msg, directory=self.docs_path) def is_dirty(self, branch_name: str | None = None) -> bool: """Check if repository path has any changes including new files. @@ -719,12 +745,15 @@ def _get_repository_name_from_git_url(remote_url: str) -> str: return matched_repository.group(1) -def create_repository_client(access_token: str | None, base_path: Path) -> Client: +def create_repository_client( + access_token: str | None, base_path: Path, charm_dir: str = "" +) -> Client: """Create a Github instance to handle communication with Github server. Args: access_token: Access token that has permissions to open a pull request. base_path: Path where local .git resides in. + charm_dir: Relative directory where the charm files are located. Raises: InputError: if invalid access token or invalid git remote URL is provided. @@ -743,4 +772,4 @@ def create_repository_client(access_token: str | None, base_path: Path) -> Clien remote_url = local_repo.remote().url repository_fullname = _get_repository_name_from_git_url(remote_url=remote_url) remote_repo = github_client.get_repo(repository_fullname) - return Client(repository=local_repo, github_repository=remote_repo) + return Client(repository=local_repo, github_repository=remote_repo, charm_dir=charm_dir) diff --git a/src/sort.py b/src/sort.py index f6cfb8f2..56e33e86 100644 --- a/src/sort.py +++ b/src/sort.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Sort items for publishing.""" diff --git a/src/types_.py b/src/types_.py index a4eac672..d07decaa 100644 --- a/src/types_.py +++ b/src/types_.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Types for uploading docs to charmhub.""" @@ -44,7 +44,8 @@ class UserInputs(typing.NamedTuple): github_access_token: A Personal Access Token(PAT) or access token with repository access. Required in migration mode. commit_sha: The SHA of the commit the action is running on. - base_branch: The main branch against which the syncs act on + base_branch: The main branch against which the syncs act on. + charm_dir: Directory the charm is located in. """ discourse: UserInputsDiscourse @@ -53,6 +54,7 @@ class UserInputs(typing.NamedTuple): github_access_token: str | None commit_sha: str base_branch: str + charm_dir: str class Metadata(typing.NamedTuple): @@ -389,6 +391,18 @@ class UpdatePageAction(_UpdateActionBase): content_change: ContentChange + def __str__(self) -> str: + """Return a formatted representation of the dataclass. + + Returns: + Formatted representation of the dataclass. + """ + return ( + f"class: {self.__class__}, level: {self.level}, path: {self.path}, " + f"navlink_change: {self.navlink_change}\n" + f"content_change:\n{self.content_change}" + ) + @dataclasses.dataclass class UpdateExternalRefAction(_UpdateActionBase): diff --git a/tests/__init__.py b/tests/__init__.py index 8d82e8f8..ef83f48c 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Tests for uploading docs to charmhub.""" diff --git a/tests/conftest.py b/tests/conftest.py index 56008595..c9067077 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Fixtures for all tests.""" diff --git a/tests/factories.py b/tests/factories.py index b1ceb332..63a902b6 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Factories for generating test data.""" @@ -337,7 +337,7 @@ class Meta: abstract = False hostname = factory.Sequence(lambda n: f"discourse/{n}") - category_id = factory.Sequence(lambda n: f"{n}") + category_id = factory.Sequence(str) api_username = factory.Sequence(lambda n: f"discourse-test-user-{n}") api_key = factory.Sequence(lambda n: f"discourse-test-key-{n}") @@ -359,6 +359,7 @@ class Meta: base_branch = DEFAULT_BRANCH dry_run = False delete_pages = False + charm_dir = "" class TableRowFactory( diff --git a/tests/integration/__init__.py b/tests/integration/__init__.py index a668c45b..a89ad696 100644 --- a/tests/integration/__init__.py +++ b/tests/integration/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. -"""Unit tests for uploading docs to charmhub.""" +"""Integration tests for uploading docs to charmhub.""" diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e49842fc..72e19e32 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Fixtures for integration tests.""" @@ -364,7 +364,6 @@ async def discourse_remove_rate_limits( settings = { "unique_posts_mins": "0", "rate_limit_create_post": "0", - "rate_limit_new_user_create_topic": "0", "rate_limit_new_user_create_post": "0", "max_topics_per_day": "1000", "max_edits_per_day": "1000", diff --git a/tests/integration/test___init__run_conflict.py b/tests/integration/test___init__run_conflict.py index d3c2626b..d979a95a 100644 --- a/tests/integration/test___init__run_conflict.py +++ b/tests/integration/test___init__run_conflict.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Integration tests for running the action where there is a merge conflict.""" @@ -20,7 +20,7 @@ from github.ContentFile import ContentFile from src import Clients, constants, exceptions, metadata, repository, run_reconcile -from src.constants import DEFAULT_BRANCH, DISCOURSE_AHEAD_TAG, DOCUMENTATION_TAG +from src.constants import DEFAULT_BRANCH, DOCUMENTATION_TAG from src.discourse import Discourse from .. import factories @@ -49,8 +49,6 @@ async def test_run_conflict( 5. docs with an index and documentation and alternate documentation file 6. docs with an index and changed documentation and alternate documentation with server changes - 7. docs with an index and changed documentation and alternate documentation with server - changes with discourse-gatekeeper/discourse-ahead-ok applied assert: then: 1. the documentation page is created 2. the documentation page is not updated @@ -58,7 +56,6 @@ async def test_run_conflict( 4. the documentation page is updated 5. the alternate documentation page is created 6. the documentation page is not updated - 6. the documentation page is updated """ document_name = "name 1" caplog.set_level(logging.INFO) @@ -77,7 +74,7 @@ async def test_run_conflict( caplog.clear() index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START}".strip(), + content=constants.NAVIGATION_TABLE_START.strip(), ) create_metadata_yaml( content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", @@ -98,7 +95,9 @@ async def test_run_conflict( reconcile_output = run_reconcile( clients=Clients(discourse=discourse_api, repository=repository_client), user_inputs=factories.UserInputsFactory( - dry_run=False, delete_pages=True, commit_sha=repository_client.current_commit + dry_run=False, + delete_pages=True, + commit_sha=repository_client.current_commit, ), ) @@ -132,7 +131,8 @@ async def test_run_conflict( repository_client.switch(DEFAULT_BRANCH).update_branch( "2. docs with a documentation file updated and discourse updated with conflicting " - "content in dry run mode" + "content in dry run mode", + directory=repository_client.docs_path, ) with pytest.raises(exceptions.InputError) as exc_info: @@ -196,7 +196,8 @@ async def test_run_conflict( discourse_api.update_topic(url=doc_url, content=doc_content_4) repository_client.switch(DEFAULT_BRANCH).update_branch( - "4. docs with a documentation file and discourse updated to resolve conflict" + "4. docs with a documentation file and discourse updated to resolve conflict", + directory=repository_client.docs_path, ) reconcile_output = run_reconcile( @@ -225,7 +226,8 @@ async def test_run_conflict( doc_file.write_text(doc_content_5 := f"# {doc_title}\ncontent 5", encoding="utf-8") repository_client.switch(DEFAULT_BRANCH).update_branch( - "5. docs with an index and documentation and alternate documentation file" + "5. docs with an index and documentation and alternate documentation file", + directory=repository_client.docs_path, ) mock_content_file.content = b64encode(doc_content_4.encode(encoding="utf-8")) @@ -260,11 +262,12 @@ async def test_run_conflict( # 6. docs with an index and changed documentation and alternate documentation with server # changes caplog.clear() - doc_file.write_text(doc_content_6 := f"# {doc_title}\ncontent 6", encoding="utf-8") + doc_file.write_text(f"# {doc_title}\ncontent 6", encoding="utf-8") repository_client.switch(DEFAULT_BRANCH).update_branch( "# 6. docs with an index and changed documentation and alternate documentation with " - "server changes" + "server changes", + directory=repository_client.docs_path, ) mock_content_file.content = b64encode(doc_content_5.encode(encoding="utf-8")) mock_alt_content_file = MagicMock(spec=ContentFile) @@ -300,34 +303,3 @@ async def test_run_conflict( assert doc_topic == doc_content_5 alt_doc_topic = discourse_api.retrieve_topic(url=alt_doc_url) assert alt_doc_topic == alt_doc_topic_content_6 - - # 7. docs with an index and changed documentation and alternate documentation with server - # changes with discourse-gatekeeper/discourse-ahead-ok applied - caplog.clear() - repository_client.tag_commit(DISCOURSE_AHEAD_TAG, repository_client.current_commit) - mock_github_repo.get_contents.side_effect = [mock_alt_content_file, mock_content_file] - - reconcile_output = run_reconcile( - clients=Clients(discourse=discourse_api, repository=repository_client), - user_inputs=factories.UserInputsFactory( - dry_run=False, delete_pages=True, commit_sha=repository_client.current_commit - ), - ) - - assert reconcile_output is not None - urls_with_actions = reconcile_output.topics - - assert len(urls_with_actions) == 3 - (alt_doc_url, _, _) = urls_with_actions.keys() - assert (urls := tuple(urls_with_actions)) == (alt_doc_url, doc_url, index_url) - assert_substrings_in_string( - chain(urls, (doc_table_line_1, alt_doc_table_line_5, "Update", "'success'")), - caplog.text, - ) - index_topic = discourse_api.retrieve_topic(url=index_url) - assert doc_table_line_1 in index_topic - assert alt_doc_table_line_5 in index_topic - doc_topic = discourse_api.retrieve_topic(url=doc_url) - assert doc_topic == doc_content_6 - alt_doc_topic = discourse_api.retrieve_topic(url=alt_doc_url) - assert alt_doc_topic == alt_doc_topic_content_6 diff --git a/tests/integration/test___init__run_migrate.py b/tests/integration/test___init__run_migrate.py index d5dc21e5..ed31b5c9 100644 --- a/tests/integration/test___init__run_migrate.py +++ b/tests/integration/test___init__run_migrate.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Integration tests for running the migrate action.""" @@ -29,7 +29,8 @@ @pytest.mark.asyncio -@pytest.mark.usefixtures("patch_create_repository_client") +@pytest.mark.usefixtures("git_repo") +@pytest.mark.parametrize("charm_id, charm_dir", [("1", ""), ("2", "charm")]) async def test_run_migrate( discourse_address: str, discourse_api: Discourse, @@ -40,6 +41,8 @@ async def test_run_migrate( mock_pull_request: PullRequest, mock_github_repo: MagicMock, monkeypatch, + charm_id: str, + charm_dir: str, ): """ arrange: given running discourse server @@ -55,7 +58,7 @@ async def test_run_migrate( 4. no operation are done """ caplog.set_level(logging.INFO) - document_name = "migration name 1" + document_name = f"migration name {charm_id}" discourse_prefix = discourse_address content_page_1 = factories.ContentPageFactory() content_page_1_url = discourse_api.create_topic( @@ -102,14 +105,20 @@ async def test_run_migrate( content=index_page_content, ) - repository_client = RepositoryClient(Repo(repository_path), mock_github_repo) + charm_path = repository_path / charm_dir + # exist_ok=True as it can be the base directory + charm_path.mkdir(exist_ok=True) + + repository_client = RepositoryClient( + Repo(repository_path), mock_github_repo, charm_dir=charm_dir + ) # 1. with no docs dir and a metadata.yaml with docs key caplog.clear() create_metadata_yaml( content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", - path=repository_path, + path=charm_path, ) repository_client.switch(DEFAULT_BRANCH).update_branch( @@ -126,7 +135,7 @@ async def test_run_migrate( ) upstream_git_repo.git.checkout(DEFAULT_BRANCH_NAME) - upstream_doc_dir = upstream_repository_path / constants.DOCUMENTATION_FOLDER_NAME + upstream_doc_dir = upstream_repository_path / charm_dir / constants.DOCUMENTATION_FOLDER_NAME assert output_migrate is not None assert output_migrate.pull_request_url == mock_pull_request.html_url assert output_migrate.action == PullRequestAction.OPENED @@ -173,7 +182,9 @@ async def test_run_migrate( output_migrate = run_migrate( Clients( discourse=discourse_api, - repository=RepositoryClient(Repo(repository_path), mock_github_repo), + repository=RepositoryClient( + Repo(repository_path), mock_github_repo, charm_dir=charm_dir + ), ), user_inputs=factories.UserInputsFactory(commit_sha=repository_client.current_commit), ) @@ -193,7 +204,9 @@ async def test_run_migrate( output_migrate = run_migrate( Clients( discourse=discourse_api, - repository=RepositoryClient(Repo(repository_path), mock_github_repo), + repository=RepositoryClient( + Repo(repository_path), mock_github_repo, charm_dir=charm_dir + ), ), user_inputs=factories.UserInputsFactory(commit_sha=repository_client.current_commit), ) @@ -237,7 +250,9 @@ def mock_edit(*args, **kwargs): # pylint: disable=W0613 output_migrate = run_migrate( Clients( discourse=discourse_api, - repository=RepositoryClient(Repo(repository_path), mock_github_repo), + repository=RepositoryClient( + Repo(repository_path), mock_github_repo, charm_dir=charm_dir + ), ), user_inputs=factories.UserInputsFactory(commit_sha=repository_client.current_commit), ) @@ -246,8 +261,8 @@ def mock_edit(*args, **kwargs): # pylint: disable=W0613 assert output_migrate.action == PullRequestAction.CLOSED assert_substrings_in_string( [ - "No community contribution found in commit", - f"Discourse is inline with {DOCUMENTATION_TAG}", + "No community contribution found", + f"discourse is inline with {DOCUMENTATION_TAG}", ], caplog.text, ) diff --git a/tests/integration/test___init__run_reconcile.py b/tests/integration/test___init__run_reconcile.py index cbfb609d..4d33339d 100644 --- a/tests/integration/test___init__run_reconcile.py +++ b/tests/integration/test___init__run_reconcile.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Integration tests for running the reconcile portion of the action.""" @@ -29,12 +29,15 @@ @pytest.mark.asyncio -@pytest.mark.usefixtures("patch_create_repository_client") +@pytest.mark.usefixtures("git_repo") +@pytest.mark.parametrize("charm_id, charm_dir", [("1", ""), ("2", "charm")]) async def test_run( discourse_api: Discourse, caplog: pytest.LogCaptureFixture, repository_path: Path, mock_github_repo: MagicMock, + charm_id: str, + charm_dir: str, ): """ arrange: given running discourse server @@ -70,16 +73,17 @@ async def test_run( 13. the documentation page is deleted 14. an index page is not updated """ - document_name = "name 1" + document_name = f"name {charm_id}" caplog.set_level(logging.INFO) - repository_client = Client(Repo(repository_path), mock_github_repo) + charm_path = repository_path / charm_dir + # exist_ok=True as it can be the base directory + charm_path.mkdir(exist_ok=True) + repository_client = Client(Repo(repository_path), mock_github_repo, charm_dir=charm_dir) repository_client.tag_commit(DOCUMENTATION_TAG, repository_client.current_commit) - create_metadata_yaml( - content=f"{metadata.METADATA_NAME_KEY}: {document_name}", path=repository_path - ) + create_metadata_yaml(content=f"{metadata.METADATA_NAME_KEY}: {document_name}", path=charm_path) repository_client.switch(DEFAULT_BRANCH).update_branch( "first commit of metadata", directory=None @@ -89,13 +93,16 @@ async def test_run( caplog.clear() index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START}".strip(), + content=constants.NAVIGATION_TABLE_START.strip(), ) create_metadata_yaml( - content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", - path=repository_path, + content=( + f"{metadata.METADATA_NAME_KEY}: name {charm_id}\n" + f"{metadata.METADATA_DOCS_KEY}: {index_url}" + ), + path=charm_path, ) - (docs_dir := repository_path / constants.DOCUMENTATION_FOLDER_NAME).mkdir() + (docs_dir := charm_path / constants.DOCUMENTATION_FOLDER_NAME).mkdir() (index_file := docs_dir / "index.md").write_text( index_content := "index content 1", encoding="utf-8" ) @@ -114,7 +121,7 @@ async def test_run( assert output_reconcile is not None assert output_reconcile.index_url == index_url index_topic = discourse_api.retrieve_topic(url=index_url) - assert index_topic == f"{constants.NAVIGATION_TABLE_START}".strip() + assert index_topic == constants.NAVIGATION_TABLE_START.strip() assert_substrings_in_string((index_url, "Update", "'skip'"), caplog.text) mock_github_repo.create_git_ref.assert_not_called() @@ -143,7 +150,8 @@ async def test_run( ) repository_client.switch(DEFAULT_BRANCH).update_branch( - "3. docs with a documentation file added in dry run mode" + "3. docs with a documentation file added in dry run mode", + directory=repository_client.docs_path, ) output_reconcile = run_reconcile( @@ -190,7 +198,8 @@ async def test_run( mock_github_repo.get_contents.return_value = mock_content_file repository_client.switch(DEFAULT_BRANCH).update_branch( - "5. docs with a documentation file updated in dry run mode" + "5. docs with a documentation file updated in dry run mode", + directory=repository_client.docs_path, ) output_reconcile = run_reconcile( @@ -239,7 +248,9 @@ async def test_run( (nested_dir := docs_dir / nested_dir_table_key).mkdir() (nested_dir / ".gitkeep").touch() - repository_client.switch(DEFAULT_BRANCH).update_branch("7. docs with a nested directory added") + repository_client.switch(DEFAULT_BRANCH).update_branch( + "7. docs with a nested directory added", directory=repository_client.docs_path + ) output_reconcile = run_reconcile( clients=Clients(discourse=discourse_api, repository=repository_client), @@ -265,7 +276,8 @@ async def test_run( ) repository_client.switch(DEFAULT_BRANCH).update_branch( - "8. docs with a documentation file added in the nested directory" + "8. docs with a documentation file added in the nested directory", + directory=repository_client.docs_path, ) output_reconcile = run_reconcile( @@ -308,7 +320,8 @@ async def test_run( ) repository_client.switch(DEFAULT_BRANCH).update_branch( - "9. docs with index file with a local contents index" + "9. docs with index file with a local contents index", + directory=repository_client.docs_path, ) output_reconcile = run_reconcile( @@ -353,7 +366,8 @@ async def test_run( nested_dir_doc_file.unlink() repository_client.switch(DEFAULT_BRANCH).update_branch( - "10. docs with the documentation file in the nested directory removed in dry run mode" + "10. docs with the documentation file in the nested directory removed in dry run mode", + directory=repository_client.docs_path, ) output_reconcile = run_reconcile( @@ -402,7 +416,9 @@ async def test_run( caplog.clear() shutil.rmtree(nested_dir) - repository_client.switch(DEFAULT_BRANCH).update_branch("12. with the nested directory removed") + repository_client.switch(DEFAULT_BRANCH).update_branch( + "12. with the nested directory removed", directory=repository_client.docs_path + ) output_reconcile = run_reconcile( clients=Clients(discourse=discourse_api, repository=repository_client), @@ -426,7 +442,7 @@ async def test_run( doc_file.unlink() repository_client.switch(DEFAULT_BRANCH).update_branch( - "13. with the documentation file removed" + "13. with the documentation file removed", directory=repository_client.docs_path ) output_reconcile = run_reconcile( @@ -452,7 +468,9 @@ async def test_run( caplog.clear() index_file.unlink() - repository_client.switch(DEFAULT_BRANCH).update_branch("14. with the index file removed") + repository_client.switch(DEFAULT_BRANCH).update_branch( + "14. with the index file removed", directory=repository_client.docs_path + ) output_reconcile = run_reconcile( clients=Clients(discourse=discourse_api, repository=repository_client), @@ -508,7 +526,7 @@ async def test_run_hidden( caplog.clear() index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START}".strip(), + content=constants.NAVIGATION_TABLE_START.strip(), ) create_metadata_yaml( content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", @@ -747,7 +765,7 @@ async def test_run_external( caplog.clear() index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START}".strip(), + content=constants.NAVIGATION_TABLE_START.strip(), ) create_metadata_yaml( content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", diff --git a/tests/integration/test_discourse.py b/tests/integration/test_discourse.py index f9c40bb9..eb2f5140 100644 --- a/tests/integration/test_discourse.py +++ b/tests/integration/test_discourse.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Integration tests for discourse.""" @@ -327,6 +327,6 @@ async def test_read_write_permission( assert alternate_user_discourse.check_topic_read_permission( url=url ), "alternate user could not read topic another user created" - assert ( - alternate_user_discourse.check_topic_write_permission(url=url) is False + assert not alternate_user_discourse.check_topic_write_permission( + url=url ), "user could write topic another user created" diff --git a/tests/integration/types.py b/tests/integration/types.py index 677d2bb6..2a97bdd7 100644 --- a/tests/integration/types.py +++ b/tests/integration/types.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Useful types for integration tests.""" diff --git a/tests/types.py b/tests/types.py index c9dd8f22..0f3610f0 100644 --- a/tests/types.py +++ b/tests/types.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Useful types for tests.""" diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py index a668c45b..8bbe3e50 100644 --- a/tests/unit/__init__.py +++ b/tests/unit/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for uploading docs to charmhub.""" diff --git a/tests/unit/action/__init__.py b/tests/unit/action/__init__.py index 4dbd5891..8320c2b9 100644 --- a/tests/unit/action/__init__.py +++ b/tests/unit/action/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for actions.""" diff --git a/tests/unit/action/test_other_actions.py b/tests/unit/action/test_other_actions.py index da2c82f5..e44235d1 100644 --- a/tests/unit/action/test_other_actions.py +++ b/tests/unit/action/test_other_actions.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for action.""" diff --git a/tests/unit/action/test_update_action.py b/tests/unit/action/test_update_action.py index 43fe3650..cbb54b57 100644 --- a/tests/unit/action/test_update_action.py +++ b/tests/unit/action/test_update_action.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for action.""" @@ -232,8 +232,9 @@ def test__update_file_navlink_content_change_discourse_error(caplog: pytest.LogC mocked_discourse = mock.MagicMock(spec=discourse.Discourse) url = "url 1" mocked_discourse.absolute_url.return_value = url + server_content = "content 1" + mocked_discourse.retrieve_topic.return_value = server_content mocked_discourse.update_topic.side_effect = (error := exceptions.DiscourseError("failed")) - server_content: str update_action = factories.UpdatePageActionFactory( level=(level := 1), path=(path := ("path 1",)), @@ -242,7 +243,7 @@ def test__update_file_navlink_content_change_discourse_error(caplog: pytest.LogC new=factories.NavlinkFactory(title="title 2", link=link), ), content_change=src_types.ContentChange( - server=(server_content := "content 1"), + server=server_content, local=(local_content := "content 2"), base=server_content, ), @@ -308,6 +309,7 @@ def test__update_file_navlink_content_change_conflict( mocked_discourse = mock.MagicMock(spec=discourse.Discourse) url = "url 1" mocked_discourse.absolute_url.return_value = url + mocked_discourse.retrieve_topic.return_value = content_change.server update_action = factories.UpdatePageActionFactory( level=(level := 1), path=(path := ("path 1",)), @@ -346,6 +348,55 @@ def test__update_file_navlink_content_change_conflict( assert_substrings_in_string(expected_reason_contents, returned_report.reason) +def test__update_file_navlink_content_change_post_check_conflict(caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a file where content has changed and mocked discourse where + the server content has changed since the conflict check was done + act: when action is passed to _update with dry_run False + assert: then an ActionError is raised. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + server_content_1 = "line 1a\nline 2\nline 3\n" + server_content_2 = "line 1b\nline 2\nline 3\n" + mocked_discourse.retrieve_topic.return_value = server_content_1 + update_action = factories.UpdatePageActionFactory( + navlink_change=factories.NavlinkChangeFactory( + old=factories.NavlinkFactory(title="title 1", link=(link := "link 1")), + new=factories.NavlinkFactory(title="title 2", link=link), + ), + content_change=src_types.ContentChange( + server=server_content_2, + local=(local_content := "line 1\nline 2\nline 3a\n"), + base=(base_content := "line 1\nline 2\nline 3\n"), + ), + ) + dry_run = False + + with pytest.raises(exceptions.ActionError) as exc: + action._update(action=update_action, discourse=mocked_discourse, dry_run=dry_run) + + assert_substrings_in_string( + (link, "has changed", "conflict check"), + str(exc), + ) + assert_substrings_in_string( + ( + f"action: {update_action}", + f"dry run: {dry_run}", + repr(base_content), + repr(server_content_2), + repr(local_content), + "content change:\n line 1\n line 2\n- line 3\n+ line 3a\n? +\n", + ), + caplog.text, + ) + assert update_action.content_change is not None + mocked_discourse.update_topic.assert_not_called() + + def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): """ arrange: given update action for a file where content has changed and mocked discourse @@ -357,7 +408,8 @@ def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): mocked_discourse = mock.MagicMock(spec=discourse.Discourse) url = "url 1" mocked_discourse.absolute_url.return_value = url - server_content: str + server_content = "line 1a\nline 2\nline 3\n" + mocked_discourse.retrieve_topic.return_value = server_content update_action = factories.UpdatePageActionFactory( level=(level := 1), path=(path := ("path 1",)), @@ -366,7 +418,7 @@ def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): new=factories.NavlinkFactory(title="title 2", link=link), ), content_change=src_types.ContentChange( - server=(server_content := "line 1a\nline 2\nline 3\n"), + server=server_content, local=(local_content := "line 1\nline 2\nline 3a\n"), base=(base_content := "line 1\nline 2\nline 3\n"), ), @@ -389,6 +441,7 @@ def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): caplog.text, ) assert update_action.content_change is not None + mocked_discourse.retrieve_topic.assert_called_once_with(url=link) mocked_discourse.update_topic.assert_called_once_with( url=link, content="line 1a\nline 2\nline 3a\n" ) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index ba9b2728..359db76f 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Fixtures for all unit tests.""" diff --git a/tests/unit/helpers.py b/tests/unit/helpers.py index 97bece3c..61007d3f 100644 --- a/tests/unit/helpers.py +++ b/tests/unit/helpers.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Helper functions for tests.""" @@ -22,6 +22,18 @@ def create_metadata_yaml(content: str, path: Path) -> None: metadata_yaml.write_text(content, encoding="utf-8") +def create_charmcraft_yaml(content: str, path: Path) -> None: + """Create the charmcraft file. + + Args: + content: The text to be written to the file. + path: The directory to create the file in. + + """ + charmcraft_yaml = path / metadata.CHARMCRAFT_FILENAME + charmcraft_yaml.write_text(content, encoding="utf-8") + + def assert_substrings_in_string(substrings: typing.Iterable[str], string: str) -> None: """Assert that a string contains substrings. diff --git a/tests/unit/test___init__.py b/tests/unit/test___init__.py index 0f05dc24..5b559d67 100644 --- a/tests/unit/test___init__.py +++ b/tests/unit/test___init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. # pylint: disable=too-many-lines """Unit tests for execution.""" @@ -11,7 +11,6 @@ from github.PullRequest import PullRequest from src import ( # GETTING_STARTED, - DOCUMENTATION_FOLDER_NAME, DOCUMENTATION_TAG, Clients, constants, @@ -23,7 +22,7 @@ types_, ) from src.clients import get_clients -from src.constants import DEFAULT_BRANCH +from src.constants import DEFAULT_BRANCH, DOCUMENTATION_FOLDER_NAME from src.metadata import METADATA_DOCS_KEY, METADATA_NAME_KEY from src.repository import DEFAULT_BRANCH_NAME from src.repository import Client as RepositoryClient @@ -36,8 +35,9 @@ # pylint: disable=protected-access +@pytest.mark.parametrize("charm_dir", ["", "charm"]) @mock.patch("github.Github.get_repo") -def test_setup_clients(get_repo_mock, git_repo_with_remote): +def test_setup_clients(get_repo_mock, git_repo_with_remote, charm_dir): """ arrange: given a local path and user_inputs act: when get_clients is called @@ -47,10 +47,12 @@ def test_setup_clients(get_repo_mock, git_repo_with_remote): path = Path(git_repo_with_remote.working_dir) - user_inputs = factories.UserInputsFactory() + user_inputs = factories.UserInputsFactory(charm_dir=charm_dir) clients = get_clients(user_inputs=user_inputs, base_path=path) assert clients.repository.base_path == path + assert clients.repository.base_charm_path == path / charm_dir + assert clients.repository.docs_path == path / charm_dir / DOCUMENTATION_FOLDER_NAME assert clients.discourse._category_id == int(user_inputs.discourse.category_id) assert clients.discourse.host == f"https://{user_inputs.discourse.hostname}" @@ -82,7 +84,7 @@ def test__run_reconcile_empty_local_server(mocked_clients): mocked_clients.discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START.strip()}", + content=constants.NAVIGATION_TABLE_START.strip(), ) assert returned_page_interactions is not None assert returned_page_interactions.topics == {url: types_.ActionResult.SUCCESS} @@ -143,7 +145,7 @@ def test__run_reconcile_local_empty_server(mocked_clients): (docs_folder := repo.base_path / "docs").mkdir() (docs_folder / "index.md").write_text(index_content := "index content\n") (docs_folder / "page.md").write_text(page_content := "page content") - repo.update_branch("new commit") + repo.update_branch("new commit", directory=repo.docs_path) user_inputs = factories.UserInputsFactory( dry_run=False, delete_pages=True, commit_sha=repo.current_commit @@ -309,7 +311,7 @@ def test__run_reconcile_local_contents_index(mocked_clients): """, encoding="utf-8", ) - repo.update_branch("new commit") + repo.update_branch("new commit", directory=repo.docs_path) user_inputs = factories.UserInputsFactory( dry_run=False, delete_pages=True, commit_sha=repo.current_commit @@ -364,7 +366,7 @@ def test__run_reconcile_hidden_item(mocked_clients): """, encoding="utf-8", ) - repo.update_branch("new commit") + repo.update_branch("new commit", directory=repo.docs_path) user_inputs = factories.UserInputsFactory( dry_run=False, delete_pages=True, commit_sha=repo.current_commit @@ -409,7 +411,7 @@ def test__run_reconcile_invalid_external_item(mocked_clients): """, encoding="utf-8", ) - repo.update_branch("new commit") + repo.update_branch("new commit", directory=repo.docs_path) user_inputs = factories.UserInputsFactory( dry_run=False, delete_pages=True, commit_sha=repo.current_commit @@ -444,7 +446,7 @@ def test__run_reconcile_external_item(mocked_clients): """, encoding="utf-8", ) - repo.update_branch("new commit") + repo.update_branch("new commit", directory=repo.docs_path) user_inputs = factories.UserInputsFactory( dry_run=False, delete_pages=True, commit_sha=repo.current_commit @@ -541,7 +543,7 @@ def test__run_reconcile_local_empty_server_error(mocked_clients): mocked_clients.discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START.strip()}", + content=constants.NAVIGATION_TABLE_START.strip(), ) assert returned_page_interactions is not None assert not returned_page_interactions.topics @@ -625,7 +627,9 @@ def test__run_reconcile_on_tag_commit(caplog, mocked_clients): (docs_folder / "index.md").write_text("index content") (docs_folder / "page.md").write_text("page content 2") - repository_client.switch(DEFAULT_BRANCH).update_branch("First commit of documentation") + repository_client.switch(DEFAULT_BRANCH).update_branch( + "First commit of documentation", directory=repository_client.docs_path + ) repository_client.tag_commit(DOCUMENTATION_TAG, repository_client.current_commit) user_inputs = factories.UserInputsFactory(commit_sha=repository_client.current_commit) @@ -931,7 +935,7 @@ def test_run_no_docs_empty_dir(mocked_clients): mocked_clients.discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{constants.NAVIGATION_TABLE_START.strip()}", + content=constants.NAVIGATION_TABLE_START.strip(), ) assert returned_page_interactions is not None assert returned_page_interactions.topics == {url: types_.ActionResult.SUCCESS} @@ -1179,6 +1183,69 @@ def test_run_migrate_same_content_local_and_server_open_pr( assert edit_call_args[0] == {"state": "closed"} +def test_run_migrate_same_content_local_and_server_tag_not_moved(caplog, mocked_clients): + """ + arrange: given a path with a metadata.yaml that has docs key and docs directory aligned + and mocked discourse (with tag one commit before main branch) + act: when run_migrate is called + assert: then nothing is done as the two versions are the compatible and tag is moved. + """ + repository_path = mocked_clients.repository.base_path + + create_metadata_yaml( + content=f"{METADATA_NAME_KEY}: name 1\n" f"{METADATA_DOCS_KEY}: https://discourse/t/docs", + path=repository_path, + ) + index_content = """Content header lorem. + + Content body.\n""" + index_table = f"""{constants.NAVIGATION_TABLE_START} + | 1 | their-path-1 | [empty-navlink]() | + | 2 | their-file-1 | [file-navlink](/file-navlink) |""" + index_page = f"{index_content}{index_table}" + navlink_page_1 = "file-navlink-content 1" + + (docs_folder := mocked_clients.repository.base_path / "docs").mkdir() + (docs_folder / "index.md").write_text( + f"{index_content}\n" + "# Contents\n\n" + "1. [empty-navlink](their-path-1)\n" + " 1. [file-navlink](their-path-1/their-file-1.md)" + ) + (docs_folder / "their-path-1").mkdir() + (docs_folder / "their-path-1" / "their-file-1.md").write_text(navlink_page_1) + + mocked_clients.repository.switch(DEFAULT_BRANCH).update_branch( + "First document version", directory=None + ) + + user_inputs = factories.UserInputsFactory(commit_sha=mocked_clients.repository.current_commit) + mocked_clients.repository.tag_commit( + DOCUMENTATION_TAG, mocked_clients.repository.current_commit + ) + + # Make a change + navlink_page_2 = "file-navlink-content 2" + (docs_folder / "their-path-1" / "their-file-1.md").write_text(navlink_page_2) + mocked_clients.repository.update_branch("A change", directory=None) + mocked_clients.discourse.retrieve_topic.side_effect = [index_page, navlink_page_2] + + caplog.clear() + with caplog.at_level(logging.INFO): + # run is repeated in unit tests / integration tests + returned_migration_reports = run_migrate( + clients=mocked_clients, user_inputs=user_inputs + ) # pylint: disable=duplicate-code + + assert returned_migration_reports is None + assert any("No community contribution found" in record.message for record in caplog.records) + mocked_clients.repository.switch(DEFAULT_BRANCH) + assert ( + mocked_clients.repository.tag_exists(DOCUMENTATION_TAG) + == mocked_clients.repository.current_commit + ) + + def test_pre_flight_checks_ok(mocked_clients): """ arrange: given a repository in a consistent state, meaning that the documentation tag is diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index c1c03f06..06c0efbc 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for check.""" @@ -8,7 +8,7 @@ import pytest -from src import check, constants, repository, types_ +from src import check, types_ from .. import factories from .helpers import assert_substrings_in_string @@ -181,14 +181,13 @@ def _test_conflicts_parameters(): The tests. """ return [ - pytest.param((), False, (), id="empty"), - pytest.param((factories.CreatePageActionFactory(),), False, (), id="single create"), - pytest.param((factories.NoopPageActionFactory(),), False, (), id="single noop"), - pytest.param((factories.DeletePageActionFactory(),), False, (), id="single delete"), - pytest.param((factories.UpdateGroupActionFactory(),), False, (), id="single group update"), + pytest.param((), (), id="empty"), + pytest.param((factories.CreatePageActionFactory(),), (), id="single create"), + pytest.param((factories.NoopPageActionFactory(),), (), id="single noop"), + pytest.param((factories.DeletePageActionFactory(),), (), id="single delete"), + pytest.param((factories.UpdateGroupActionFactory(),), (), id="single group update"), pytest.param( (factories.UpdateExternalRefActionFactory(),), - False, (), id="single external ref update", ), @@ -198,7 +197,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base=None, server="a", local="a") ), ), - False, (), id="single update no base content same", ), @@ -208,7 +206,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base=None, server="a", local="b") ), ), - False, ( ExpectedProblem( path="/".join(action_1.path), @@ -229,7 +226,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base="a", server="a", local="a") ), ), - False, (), id="single update no conflict", ), @@ -239,7 +235,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base="a", server="b", local="c") ), ), - False, ( ExpectedProblem( path="/".join(action_1.path), @@ -253,7 +248,6 @@ def _test_conflicts_parameters(): ), pytest.param( (factories.NoopPageActionFactory(), factories.NoopPageActionFactory()), - False, (), id="multiple actions no problems", ), @@ -264,7 +258,6 @@ def _test_conflicts_parameters(): ), factories.NoopPageActionFactory(), ), - False, ( ExpectedProblem( path="/".join(action_1.path), @@ -283,7 +276,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base="x", server="y", local="z") ), ), - False, ( ExpectedProblem( path="/".join(action_2.path), @@ -304,7 +296,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base="x", server="y", local="z") ), ), - False, ( ExpectedProblem( path="/".join(action_1.path), @@ -332,7 +323,6 @@ def _test_conflicts_parameters(): content_change=types_.ContentChange(base="x", server="x", local="y") ), ), - False, ( ExpectedProblem( path="/".join(action_2.path), @@ -340,28 +330,11 @@ def _test_conflicts_parameters(): "detected", "unmerged", "community contributions", - constants.DISCOURSE_AHEAD_TAG, "/".join(action_1.path), "/".join(action_2.path), ), ), ), - id=( - "multiple actions one has base and local other has local and server diff, tag not " - "applied" - ), - ), - pytest.param( - ( - action_1 := factories.UpdatePageActionFactory( - content_change=types_.ContentChange(base="a", server="b", local="a") - ), - action_2 := factories.UpdatePageActionFactory( - content_change=types_.ContentChange(base="x", server="x", local="y") - ), - ), - True, - (), id=( "multiple actions one has base and local other has local and server diff, tag " "applied" @@ -371,15 +344,13 @@ def _test_conflicts_parameters(): @pytest.mark.parametrize( - "actions, is_tagged, expected_problems", + "actions, expected_problems", _test_conflicts_parameters(), ) def test_conflicts( actions: tuple[types_.AnyAction, ...], - is_tagged: bool, expected_problems: tuple[ExpectedProblem], caplog: pytest.LogCaptureFixture, - repository_client: repository.Client, ): """ arrange: given actions @@ -387,19 +358,8 @@ def test_conflicts( assert: then the expected problems are yielded. """ caplog.set_level(logging.INFO) - if is_tagged: - repository_client.tag_commit( - tag_name=constants.DISCOURSE_AHEAD_TAG, commit_sha=repository_client.current_commit - ) - user_inputs = factories.UserInputsFactory(commit_sha=repository_client.current_commit) - returned_problems = tuple( - check.conflicts( - actions=actions, - repository=repository_client, - user_inputs=user_inputs, - ) - ) + returned_problems = tuple(check.conflicts(actions=actions)) assert len(returned_problems) == len(expected_problems) for returned_problem, expected_problem in zip(returned_problems, expected_problems): diff --git a/tests/unit/test_commit.py b/tests/unit/test_commit.py index b0b620bd..5e27d2fb 100644 --- a/tests/unit/test_commit.py +++ b/tests/unit/test_commit.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for commit module.""" @@ -6,6 +6,7 @@ # Need access to protected functions for testing # pylint: disable=protected-access +import typing from pathlib import Path from src import commit @@ -163,9 +164,10 @@ def test_parse_git_show_copied(repository_client: Client): ).update_branch(commit_msg="commit-1", push=False, directory=None) (repository_path / file).rename(repository_path / (new_file := Path("other_file.text"))) repository_client.update_branch(commit_msg="commit-2", push=False, directory=None) - show_output: str = repository_client._git_repo.git.show("--name-status") # Change renamed to copied, it isn't clear how to make git think a file was copied - show_output = show_output.replace("R100", "C100") + show_output = typing.cast(str, repository_client._git_repo.git.show("--name-status")).replace( + "R100", "C100" + ) commit_files = tuple(commit.parse_git_show(show_output, repository_path=repository_path)) diff --git a/tests/unit/test_content.py b/tests/unit/test_content.py index 0e37f92a..b70a1788 100644 --- a/tests/unit/test_content.py +++ b/tests/unit/test_content.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for content.""" diff --git a/tests/unit/test_discourse.py b/tests/unit/test_discourse.py index 8fb69d98..50d0d22b 100644 --- a/tests/unit/test_discourse.py +++ b/tests/unit/test_discourse.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for discourse.""" diff --git a/tests/unit/test_docs_directory.py b/tests/unit/test_docs_directory.py index bf8c8099..c7c281b7 100644 --- a/tests/unit/test_docs_directory.py +++ b/tests/unit/test_docs_directory.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for docs directory module.""" diff --git a/tests/unit/test_download.py b/tests/unit/test_download.py index 221739f6..58639149 100644 --- a/tests/unit/test_download.py +++ b/tests/unit/test_download.py @@ -1,11 +1,11 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for download.""" import pytest -from src import DOCUMENTATION_FOLDER_NAME, DOCUMENTATION_TAG, constants +from src import DOCUMENTATION_TAG, constants from src.download import recreate_docs from src.metadata import METADATA_DOCS_KEY, METADATA_NAME_KEY @@ -40,9 +40,14 @@ def test_recreate_docs( recreate_docs(mocked_clients, DOCUMENTATION_TAG) - assert (index_file := repository_path / DOCUMENTATION_FOLDER_NAME / "index.md").is_file() assert ( - path_file := repository_path / DOCUMENTATION_FOLDER_NAME / "page-path-1" / "page-file-1.md" + index_file := repository_path / constants.DOCUMENTATION_FOLDER_NAME / "index.md" + ).is_file() + assert ( + path_file := repository_path + / constants.DOCUMENTATION_FOLDER_NAME + / "page-path-1" + / "page-file-1.md" ).is_file() assert index_file.read_text(encoding="utf-8") == ( f"{index_content}\n" diff --git a/tests/unit/test_index.py b/tests/unit/test_index.py index ebcda847..8f370d1b 100644 --- a/tests/unit/test_index.py +++ b/tests/unit/test_index.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for index module.""" @@ -23,7 +23,7 @@ def test__read_docs_index_docs_directory_missing(tmp_path: Path): act: when _read_docs_index is called with the directory assert: then None is returned. """ - returned_content = index._read_docs_index(base_path=tmp_path) + returned_content = index._read_docs_index(docs_path=tmp_path) assert returned_content is None @@ -37,7 +37,7 @@ def test__read_docs_index_index_file_missing(tmp_path: Path): docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME docs_directory.mkdir() - returned_content = index._read_docs_index(base_path=tmp_path) + returned_content = index._read_docs_index(docs_path=docs_directory) assert returned_content is None @@ -48,7 +48,8 @@ def test__read_docs_index_index_file(index_file_content: str, tmp_path: Path): act: when _read_docs_index is called with the directory assert: then the index file content is returned. """ - returned_content = index._read_docs_index(base_path=tmp_path) + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME + returned_content = index._read_docs_index(docs_path=docs_directory) assert returned_content == index_file_content @@ -63,9 +64,10 @@ def test_get_metadata_yaml_retrieve_discourse_error(tmp_path: Path): meta = types_.Metadata(name="name", docs="http://server/index-page") mocked_server_client = mock.MagicMock(spec=discourse.Discourse) mocked_server_client.retrieve_topic.side_effect = DiscourseError + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME with pytest.raises(ServerError) as exc_info: - index.get(metadata=meta, base_path=tmp_path, server_client=mocked_server_client) + index.get(metadata=meta, docs_path=docs_directory, server_client=mocked_server_client) assert_substrings_in_string(("index page", "retrieval", "failed"), str(exc_info.value).lower()) @@ -83,9 +85,10 @@ def test_get_metadata_yaml_retrieve_local_and_server(tmp_path: Path, index_file_ meta = types_.Metadata(name=name, docs=url) mocked_server_client = mock.MagicMock(spec=discourse.Discourse) mocked_server_client.retrieve_topic.return_value = (content := "content 2") + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME returned_index = index.get( - metadata=meta, base_path=tmp_path, server_client=mocked_server_client + metadata=meta, docs_path=docs_directory, server_client=mocked_server_client ) assert returned_index.server is not None @@ -106,9 +109,10 @@ def test_get_metadata_yaml_retrieve_empty(tmp_path: Path): name = "name 1" meta = types_.Metadata(name=name, docs=None) mocked_server_client = mock.MagicMock(spec=discourse.Discourse) + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME returned_index = index.get( - metadata=meta, base_path=tmp_path, server_client=mocked_server_client + metadata=meta, docs_path=docs_directory, server_client=mocked_server_client ) assert returned_index.server is None diff --git a/tests/unit/test_index_contents_get.py b/tests/unit/test_index_contents_get.py index 048d4100..600668ef 100644 --- a/tests/unit/test_index_contents_get.py +++ b/tests/unit/test_index_contents_get.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for index module get_contents function.""" diff --git a/tests/unit/test_index_contents_hierarchy.py b/tests/unit/test_index_contents_hierarchy.py index 788645fb..a0c47891 100644 --- a/tests/unit/test_index_contents_hierarchy.py +++ b/tests/unit/test_index_contents_hierarchy.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for index module _calculate_contents_hierarchy function.""" diff --git a/tests/unit/test_index_contents_parse.py b/tests/unit/test_index_contents_parse.py index c2b55246..dea675c4 100644 --- a/tests/unit/test_index_contents_parse.py +++ b/tests/unit/test_index_contents_parse.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for index module _get_contents_parsed_items function.""" diff --git a/tests/unit/test_metadata.py b/tests/unit/test_metadata.py index 7e2988c5..dcaf8041 100644 --- a/tests/unit/test_metadata.py +++ b/tests/unit/test_metadata.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for metadata module.""" @@ -9,7 +9,7 @@ from src import exceptions, metadata, types_ -from .helpers import assert_substrings_in_string, create_metadata_yaml +from .helpers import assert_substrings_in_string, create_charmcraft_yaml, create_metadata_yaml def test_get_yaml_missing(tmp_path: Path): @@ -22,6 +22,7 @@ def test_get_yaml_missing(tmp_path: Path): metadata.get(path=tmp_path) assert metadata.METADATA_FILENAME in str(exc_info.value).lower() + assert metadata.CHARMCRAFT_FILENAME in str(exc_info.value).lower() @pytest.mark.parametrize( @@ -34,7 +35,7 @@ def test_get_yaml_missing(tmp_path: Path): pytest.param("value 1", ("not", "mapping", metadata.METADATA_FILENAME), id="not dict"), ], ) -def test_get_yaml_malformed( +def test_get_metadata_yaml_malformed( metadata_yaml_content: str, expected_error_msg_contents: tuple[str, ...], tmp_path: Path ): """ @@ -87,26 +88,34 @@ def test_get_invalid_metadata( assert_substrings_in_string(expected_error_msg_contents, str(exc_info.value).lower()) -# pylint currently does not understand walrus operator perfectly yet -# pylint: disable=unused-variable,undefined-variable -@pytest.mark.parametrize( - "metadata_yaml_content, expected_metadata", - [ +def _get_metadata_parameters(): + """Generate parameters for the metadata_parameters test. + + Returns: + The test parameters, the first item of each element is the metadata content, + the second the expected types_.Metadata and the third the test id. + """ + name = "name" + docs = "https://discourse.charmhub.io/t/index/1" + return [ pytest.param( - f"{metadata.METADATA_NAME_KEY}: {(name := 'name')}", + f"{metadata.METADATA_NAME_KEY}: {name}", types_.Metadata(name=name, docs=None), id="name only", ), pytest.param( - f"{metadata.METADATA_NAME_KEY}: {name}\n" - f"{metadata.METADATA_DOCS_KEY}: " - f"{(docs := 'https://discourse.charmhub.io/t/index/1')}", + f"{metadata.METADATA_NAME_KEY}: {name}\n" f"{metadata.METADATA_DOCS_KEY}: " f"{docs}", types_.Metadata(name=name, docs=docs), id="name and docs", ), - ], + ] + + +@pytest.mark.parametrize( + "metadata_yaml_content, expected_metadata", + _get_metadata_parameters(), ) -def test_get( +def test_get_metadata( metadata_yaml_content: str, expected_metadata: types_.Metadata, tmp_path: Path, @@ -124,3 +133,118 @@ def test_get( data = metadata.get(path=tmp_path) assert data == expected_metadata + + +@pytest.mark.parametrize( + "charmcraft_yaml_content, expected_error_msg_contents", + [ + pytest.param("", ("empty", metadata.CHARMCRAFT_FILENAME), id="malformed"), + pytest.param( + "malformed: yaml:", ("malformed", metadata.CHARMCRAFT_FILENAME), id="malformed" + ), + pytest.param("value 1", ("not", "mapping", metadata.CHARMCRAFT_FILENAME), id="not dict"), + ], +) +def test_get_charmcraft_yaml_malformed( + charmcraft_yaml_content: str, expected_error_msg_contents: tuple[str, ...], tmp_path: Path +): + """ + arrange: given directory with charmcraft.yaml that is malformed + act: when get is called with the directory + assert: then InputError is raised. + """ + create_charmcraft_yaml(content=charmcraft_yaml_content, path=tmp_path) + + with pytest.raises(exceptions.InputError) as exc_info: + metadata.get(path=tmp_path) + + assert_substrings_in_string(expected_error_msg_contents, str(exc_info.value).lower()) + + +@pytest.mark.parametrize( + "charmcraft_yaml_content, expected_error_msg_contents", + [ + pytest.param("key: value", ("could not find", metadata.CHARMCRAFT_NAME_KEY)), + pytest.param( + f"{metadata.CHARMCRAFT_NAME_KEY}: 1", + ( + "invalid value for", + metadata.CHARMCRAFT_NAME_KEY, + ), + ), + pytest.param( + f"{metadata.CHARMCRAFT_NAME_KEY}: name\n{metadata.CHARMCRAFT_LINKS_KEY}: 1", + ("invalid value for", metadata.CHARMCRAFT_LINKS_KEY), + ), + pytest.param( + ( + f"{metadata.CHARMCRAFT_NAME_KEY}: name\n" + f"{metadata.CHARMCRAFT_LINKS_KEY}:\n {metadata.CHARMCRAFT_LINKS_DOCS_KEY}: 1" + ), + ("invalid value for", metadata.CHARMCRAFT_LINKS_DOCS_KEY), + ), + ], +) +def test_get_invalid_charmcraft( + charmcraft_yaml_content: str, expected_error_msg_contents: tuple[str, ...], tmp_path: Path +): + """ + arrange: given charmcraft with missing required keys or invalid value + act: when get is called with the directory + assert: InputError is raised with missing keys info. + """ + create_charmcraft_yaml(content=charmcraft_yaml_content, path=tmp_path) + + with pytest.raises(exceptions.InputError) as exc_info: + metadata.get(path=tmp_path) + + assert_substrings_in_string(expected_error_msg_contents, str(exc_info.value).lower()) + + +def _get_charmcraft_parameters(): + """Generate parameters for the get_charmcraft test. + + Returns: + The test parameters, the first item of each element is the charmcraft content, + the second the expected types_.Metadata and the third the test id. + """ + name = "name" + docs = "https://discourse.charmhub.io/t/index/1" + return [ + pytest.param( + f"{metadata.CHARMCRAFT_NAME_KEY}: {name}", + types_.Metadata(name=name, docs=None), + id="name only", + ), + pytest.param( + f"{metadata.CHARMCRAFT_NAME_KEY}: {name}\n" + f"{metadata.CHARMCRAFT_LINKS_KEY}:\n" + f" {metadata.CHARMCRAFT_LINKS_DOCS_KEY}: " + f"{docs}", + types_.Metadata(name=name, docs=docs), + id="name and docs", + ), + ] + + +@pytest.mark.parametrize( + "charmcraft_yaml_content, expected_metadata", _get_charmcraft_parameters() +) +def test_get_charmcraft( + charmcraft_yaml_content: str, + expected_metadata: types_.Metadata, + tmp_path: Path, +): + """ + arrange: given directory with metadata.yaml with valid metadata yaml + act: when get is called with the directory + assert: then file contents are returned as a dictionary. + """ + create_charmcraft_yaml( + content=charmcraft_yaml_content, + path=tmp_path, + ) + + data = metadata.get(path=tmp_path) + + assert data == expected_metadata diff --git a/tests/unit/test_migration/__init__.py b/tests/unit/test_migration/__init__.py index 6b3fa463..b30dbe9e 100644 --- a/tests/unit/test_migration/__init__.py +++ b/tests/unit/test_migration/__init__.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for migartion module.""" diff --git a/tests/unit/test_migration/test_private.py b/tests/unit/test_migration/test_private.py index b77ae3f0..5ad8a70a 100644 --- a/tests/unit/test_migration/test_private.py +++ b/tests/unit/test_migration/test_private.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for private functions of migration module.""" diff --git a/tests/unit/test_migration/test_public.py b/tests/unit/test_migration/test_public.py index 94475435..7fd85d8e 100644 --- a/tests/unit/test_migration/test_public.py +++ b/tests/unit/test_migration/test_public.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for public functions in migration module.""" diff --git a/tests/unit/test_navigation_table.py b/tests/unit/test_navigation_table.py index c233c897..baf050fa 100644 --- a/tests/unit/test_navigation_table.py +++ b/tests/unit/test_navigation_table.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for navigation table module.""" diff --git a/tests/unit/test_reconcile.py b/tests/unit/test_reconcile.py index ffb97374..e6e374a1 100644 --- a/tests/unit/test_reconcile.py +++ b/tests/unit/test_reconcile.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for reconcile module.""" @@ -1184,7 +1184,7 @@ def test_run( # pylint: disable=too-many-arguments ), (), types_.CreateIndexAction( - title=local_title, content=f"{constants.NAVIGATION_TABLE_START.strip()}" + title=local_title, content=constants.NAVIGATION_TABLE_START.strip() ), id="empty local only empty rows", ), diff --git a/tests/unit/test_repository.py b/tests/unit/test_repository.py index a4ff1fee..b85a8a2d 100644 --- a/tests/unit/test_repository.py +++ b/tests/unit/test_repository.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for git.""" @@ -144,7 +144,7 @@ def test_commit_in_branch_check(repository_client, docs_path): test_branch ) as repo: (docs_path / "placeholder.md").touch() - repo.update_branch("commit of placeholder") + repo.update_branch("commit of placeholder", directory=repository_client.docs_path) assert repo.is_commit_in_branch(repo.current_commit) assert repo.is_commit_in_branch(repo.current_commit, test_branch) assert not repo.is_commit_in_branch(repo.current_commit, DEFAULT_BRANCH) @@ -233,7 +233,7 @@ def test_create_branch( repository_client.switch(DEFAULT_BRANCH).create_branch(branch_name=branch_name).switch( branch_name - ).update_branch(commit_msg="commit-1", push=True) + ).update_branch(commit_msg="commit-1", push=True, directory=repository_client.docs_path) # mypy false positive in lib due to getter/setter not being next to each other. assert any( @@ -268,7 +268,7 @@ def test_create_branch_checkout_clash_default( repository_client.switch(DEFAULT_BRANCH).create_branch(branch_name=branch_name).switch( branch_name - ).update_branch(commit_msg="commit-1", push=True) + ).update_branch(directory=repository_client.docs_path, commit_msg="commit-1", push=True) assert upstream_git_repo.git.ls_tree("-r", branch_name, "--name-only") @@ -291,7 +291,7 @@ def test_create_branch_checkout_clash_created( repository_client.switch(DEFAULT_BRANCH).create_branch(branch_name=branch_name).switch( branch_name - ).update_branch(commit_msg="commit-1", push=True) + ).update_branch(directory=repository_client.docs_path, commit_msg="commit-1", push=True) assert upstream_git_repo.git.ls_tree("-r", branch_name, "--name-only") @@ -377,7 +377,9 @@ def test_tag_commit_tag_update(repository_client: Client, upstream_git_repo, doc (docs_path / "placeholder.md").touch() - repository_client.switch(DEFAULT_BRANCH).update_branch("my new commit") + repository_client.switch(DEFAULT_BRANCH).update_branch( + "my new commit", directory=repository_client.docs_path + ) repository_client.tag_commit(DOCUMENTATION_TAG, repository_client.current_commit) @@ -408,7 +410,7 @@ def test_tag_other_commit(repository_client: Client, docs_path: Path): (docs_path / "placeholder.md").touch() - repo.update_branch("my new commit") + repo.update_branch("my new commit", directory=repository_client.docs_path) new_hash = repo.current_commit @@ -711,6 +713,33 @@ def test_create_repository_client( assert isinstance(returned_client, repository.Client) +def test_create_repository_client_with_charm_dir( + monkeypatch: pytest.MonkeyPatch, + git_repo_with_remote: Repo, + repository_path: Path, + mock_github_repo: Repository, +): + """ + arrange: given valid repository path and a valid access_token and a mocked github client + with a customised charm_dir + act: when create_repository_client is called + assert: RepositoryClient is returned with correct charm paths and docs paths + """ + _ = git_repo_with_remote + + test_token = secrets.token_hex(16) + mock_github_client = mock.MagicMock(spec=Github) + mock_github_client.get_repo.returns = mock_github_repo + monkeypatch.setattr(repository, "Github", mock_github_client) + + returned_client = repository.create_repository_client( + access_token=test_token, base_path=repository_path, charm_dir="charm" + ) + + assert returned_client.base_charm_path == returned_client.base_path / "charm" + assert returned_client.docs_path == returned_client.base_charm_path / DOCUMENTATION_FOLDER_NAME + + @pytest.mark.parametrize( "folder", [ @@ -826,7 +855,7 @@ def test_commit_file_outside_of_folder(repository_client, upstream_git_repo, doc assert (Path(upstream_git_repo.working_dir) / directory / file2).exists() # Locally the repository is dirty only when directory is set to None - assert not repository_client.get_summary().is_dirty + assert not repository_client.get_summary(directory=directory).is_dirty assert repository_client.get_summary(directory=None).is_dirty @@ -854,7 +883,7 @@ def test_repository_pull_default_branch( (docs_path / "filler-file-1").touch() - repository_client.update_branch("commit 1") + repository_client.update_branch("commit 1", directory=repository_client.docs_path) upstream_git_repo.git.checkout(branch_name) first_hash = upstream_git_repo.head.ref.commit.hexsha @@ -882,7 +911,7 @@ def test_repository_pull_other_branch( (docs_path / "filler-file-1").touch() - repository_client.update_branch("commit 1") + repository_client.update_branch("commit 1", directory=repository_client.docs_path) upstream_git_repo.git.checkout(branch_name) first_hash = upstream_git_repo.head.ref.commit.hexsha @@ -1018,7 +1047,7 @@ def test_update_branch_unknown_error(monkeypatch, repository_client: Client): monkeypatch.setattr(repository_client._git_repo, "is_dirty", lambda *_args, **_kwargs: True) with pytest.raises(RepositoryClientError) as exc: - repository_client.update_branch("my-message") + repository_client.update_branch("my-message", directory=repository_client.docs_path) assert_substrings_in_string(("unexpected error updating branch"), str(exc.value).lower()) @@ -1041,7 +1070,7 @@ def test_update_branch_github_api_git_error(monkeypatch, repository_client: Clie monkeypatch.setattr(repository_client._git_repo, "is_dirty", lambda *_args, **_kwargs: True) with pytest.raises(RepositoryClientError) as exc: - repository_client.update_branch("my-message") + repository_client.update_branch("my-message", directory=repository_client.docs_path) assert_substrings_in_string(("unexpected error updating branch"), str(exc.value).lower()) @@ -1072,7 +1101,7 @@ def test_update_branch_github_api_github_error( monkeypatch.setattr(repository_client._git_repo, "is_dirty", lambda *_args, **_kwargs: True) with pytest.raises(RepositoryClientError) as exc: - repository_client.update_branch("my-message") + repository_client.update_branch("my-message", directory=repository_client.docs_path) assert_substrings_in_string(("unexpected error updating branch"), str(exc.value).lower()) @@ -1105,7 +1134,7 @@ def test_update_branch_github_api( monkeypatch.setattr(repository_client._git_repo, "git", mock_git_repository) monkeypatch.setattr(repository_client._git_repo, "is_dirty", lambda *_args, **_kwargs: True) - repository_client.update_branch("my-message") + repository_client.update_branch("my-message", directory=repository_client.docs_path) # Check that the branch.edit was called, more detailed checks are in # test__github_client_push_single diff --git a/tests/unit/test_sort.py b/tests/unit/test_sort.py index d0115f88..16898251 100644 --- a/tests/unit/test_sort.py +++ b/tests/unit/test_sort.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for sort module.""" @@ -603,9 +603,11 @@ def test_using_contents_index( for path_info in path_infos ) expected_items = tuple( - change_path_info_attrs(path_info=path_info, local_path=tmp_path / path_info.local_path) - if isinstance(path_info, types_.PathInfo) - else path_info + ( + change_path_info_attrs(path_info=path_info, local_path=tmp_path / path_info.local_path) + if isinstance(path_info, types_.PathInfo) + else path_info + ) for path_info in expected_items ) diff --git a/tests/unit/test_types_.py b/tests/unit/test_types_.py index 8c16dc82..e526d1dd 100644 --- a/tests/unit/test_types_.py +++ b/tests/unit/test_types_.py @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. """Unit tests for types module.""" diff --git a/tox.ini b/tox.ini index 7b05b61e..4aafdcf3 100644 --- a/tox.ini +++ b/tox.ini @@ -1,4 +1,4 @@ -# Copyright 2023 Canonical Ltd. +# Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. [tox]