From f6aa70c5b35713ace538d3510c937381b77a7315 Mon Sep 17 00:00:00 2001 From: JerrySentry Date: Wed, 16 Oct 2024 11:40:24 +0900 Subject: [PATCH 1/3] add support for comments in global yaml --- .../owner/interactors/set_yaml_on_owner.py | 2 ++ .../interactors/tests/test_set_yaml_on_owner.py | 14 ++++++++++++-- graphql_api/types/owner/owner.py | 2 +- services/tests/test_yaml.py | 12 ++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py index 0aa633a557..9b690d0020 100644 --- a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py +++ b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py @@ -54,5 +54,7 @@ def execute(self, username, yaml_input): self.owner = self.get_owner(username) self.authorize() self.owner.yaml = self.convert_yaml_to_dict(yaml_input) + if self.owner.yaml: + self.owner.yaml["to_string"] = yaml_input self.owner.save() return self.owner diff --git a/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py b/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py index f24014a66f..1f47d936df 100644 --- a/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py +++ b/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py @@ -69,7 +69,12 @@ async def test_user_is_part_of_org_and_yaml_is_good(self): ) # check the interactor returns the right owner assert owner_updated.ownerid == self.org.ownerid - assert owner_updated.yaml == {"codecov": {"require_ci_to_pass": True}} + assert owner_updated.yaml == { + "codecov": { + "require_ci_to_pass": True, + }, + "to_string": "\n" "codecov:\n" " require_ci_to_pass: yes\n", + } async def test_user_is_part_of_org_and_yaml_has_quotes(self): owner_updated = await self.execute( @@ -77,7 +82,12 @@ async def test_user_is_part_of_org_and_yaml_has_quotes(self): ) # check the interactor returns the right owner assert owner_updated.ownerid == self.org.ownerid - assert owner_updated.yaml == {"codecov": {"bot": "codecov"}} + assert owner_updated.yaml == { + "codecov": { + "bot": "codecov", + }, + "to_string": "\n" "codecov:\n" " bot: 'codecov'\n", + } async def test_user_is_part_of_org_and_yaml_is_empty(self): owner_updated = await self.execute(self.current_owner, self.org.username, "") diff --git a/graphql_api/types/owner/owner.py b/graphql_api/types/owner/owner.py index 922c2a25b9..2ef7e3fff5 100644 --- a/graphql_api/types/owner/owner.py +++ b/graphql_api/types/owner/owner.py @@ -93,7 +93,7 @@ def resolve_yaml(owner: Owner, info: GraphQLResolveInfo): current_owner = info.context["request"].current_owner if not current_user_part_of_org(current_owner, owner): return - return yaml.dump(owner.yaml) + return owner.yaml.get("to_string", yaml.dump(owner.yaml)) @owner_bindable.field("plan") diff --git a/services/tests/test_yaml.py b/services/tests/test_yaml.py index aff1fa5692..7242a341ab 100644 --- a/services/tests/test_yaml.py +++ b/services/tests/test_yaml.py @@ -56,3 +56,15 @@ def test_when_commit_has_yaml_with_owner(self, mock_fetch_yaml): """ config = yaml.final_commit_yaml(self.commit, self.org) assert config["codecov"]["require_ci_to_pass"] is False + + @patch("services.yaml.fetch_current_yaml_from_provider_via_reference") + def test_when_commit_has_reserved_to_string_key(self, mock_fetch_yaml): + mock_fetch_yaml.return_value = """ + codecov: + notify: + require_ci_to_pass: no + to_string: hello + """ + config = yaml.final_commit_yaml(self.commit, self.org) + assert config.get("to_string") is None + assert "to_string" not in config.to_dict() From ba7e35010fa61e34b626568e3e04c83d19e9c4be Mon Sep 17 00:00:00 2001 From: JerrySentry Date: Thu, 17 Oct 2024 03:05:22 +0900 Subject: [PATCH 2/3] add typing 1 --- .../owner/interactors/set_yaml_on_owner.py | 11 ++-- graphql_api/types/owner/owner.py | 57 +++++++++++-------- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py index 9b690d0020..415cc9bf44 100644 --- a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py +++ b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py @@ -1,4 +1,5 @@ import html +from typing import Optional import yaml from shared.validation.exceptions import InvalidYamlException @@ -17,21 +18,21 @@ class SetYamlOnOwnerInteractor(BaseInteractor): - def validate(self): + def validate(self) -> None: if not self.current_user.is_authenticated: raise Unauthenticated() - def authorize(self): + def authorize(self) -> None: if not current_user_part_of_org(self.current_owner, self.owner): raise Unauthorized() - def get_owner(self, username): + def get_owner(self, username: str) -> Owner: try: return Owner.objects.get(username=username, service=self.service) except Owner.DoesNotExist: raise NotFound() - def convert_yaml_to_dict(self, yaml_input): + def convert_yaml_to_dict(self, yaml_input: str) -> Optional[dict]: yaml_safe = html.escape(yaml_input, quote=False) try: yaml_dict = yaml.safe_load(yaml_safe) @@ -49,7 +50,7 @@ def convert_yaml_to_dict(self, yaml_input): raise ValidationError(message) @sync_to_async - def execute(self, username, yaml_input): + def execute(self, username: str, yaml_input: str) -> Owner: self.validate() self.owner = self.get_owner(username) self.authorize() diff --git a/graphql_api/types/owner/owner.py b/graphql_api/types/owner/owner.py index 2ef7e3fff5..aea89daadc 100644 --- a/graphql_api/types/owner/owner.py +++ b/graphql_api/types/owner/owner.py @@ -1,6 +1,6 @@ from datetime import datetime from hashlib import sha1 -from typing import Iterable, List, Optional +from typing import Any, Iterable, List, Optional import shared.rate_limits as rate_limits import stripe @@ -26,6 +26,7 @@ ) from graphql_api.helpers.ariadne import ariadne_load_local_graphql from graphql_api.helpers.connection import ( + Connection, build_connection_graphql, queryset_to_connection, ) @@ -38,7 +39,7 @@ from services.profiling import ProfilingSummary from services.redis_configuration import get_redis_connection from timeseries.helpers import fill_sparse_measurements -from timeseries.models import Interval, MeasurementSummary +from timeseries.models import Interval from utils.config import get_config owner = ariadne_load_local_graphql(__file__, "owner.graphql") @@ -51,12 +52,12 @@ @convert_kwargs_to_snake_case def resolve_repositories( owner: Owner, - info, - filters=None, - ordering=RepositoryOrdering.ID, - ordering_direction=OrderingDirection.ASC, - **kwargs, -): + info: GraphQLResolveInfo, + filters: Optional[dict] = None, + ordering: Optional[RepositoryOrdering] = RepositoryOrdering.ID, + ordering_direction: Optional[OrderingDirection] = OrderingDirection.ASC, + **kwargs: Any, +) -> Connection: current_owner = info.context["request"].current_owner okta_account_auths: list[int] = info.context["request"].session.get( OKTA_SIGNED_IN_ACCOUNTS_SESSION_KEY, [] @@ -81,18 +82,18 @@ def resolve_repositories( @owner_bindable.field("isCurrentUserPartOfOrg") @sync_to_async -def resolve_is_current_user_part_of_org(owner, info: GraphQLResolveInfo): +def resolve_is_current_user_part_of_org(owner: Owner, info: GraphQLResolveInfo) -> bool: current_owner = info.context["request"].current_owner return current_user_part_of_org(current_owner, owner) @owner_bindable.field("yaml") -def resolve_yaml(owner: Owner, info: GraphQLResolveInfo): +def resolve_yaml(owner: Owner, info: GraphQLResolveInfo) -> Optional[str]: if owner.yaml is None: - return + return None current_owner = info.context["request"].current_owner if not current_user_part_of_org(current_owner, owner): - return + return None return owner.yaml.get("to_string", yaml.dump(owner.yaml)) @@ -134,7 +135,9 @@ def resolve_ownerid(owner: Owner, info: GraphQLResolveInfo) -> int: @owner_bindable.field("repository") -async def resolve_repository(owner: Owner, info, name): +async def resolve_repository( + owner: Owner, info: GraphQLResolveInfo, name: str +) -> Repository | NotFoundError: command = info.context["executor"].get_command("repository") okta_authenticated_accounts: list[int] = info.context["request"].session.get( OKTA_SIGNED_IN_ACCOUNTS_SESSION_KEY, [] @@ -174,14 +177,16 @@ async def resolve_repository(owner: Owner, info, name): @owner_bindable.field("numberOfUploads") @require_part_of_org -async def resolve_number_of_uploads(owner: Owner, info, **kwargs): +async def resolve_number_of_uploads( + owner: Owner, info: GraphQLResolveInfo, **kwargs: Any +) -> int: command = info.context["executor"].get_command("owner") return await command.get_uploads_number_per_user(owner) @owner_bindable.field("isAdmin") @require_part_of_org -def resolve_is_current_user_an_admin(owner: Owner, info: GraphQLResolveInfo): +def resolve_is_current_user_an_admin(owner: Owner, info: GraphQLResolveInfo) -> bool: current_owner = info.context["request"].current_owner command = info.context["executor"].get_command("owner") return command.get_is_current_user_an_admin(owner, current_owner) @@ -189,14 +194,16 @@ def resolve_is_current_user_an_admin(owner: Owner, info: GraphQLResolveInfo): @owner_bindable.field("hashOwnerid") @require_part_of_org -def resolve_hash_ownerid(owner: Owner, info: GraphQLResolveInfo): +def resolve_hash_ownerid(owner: Owner, info: GraphQLResolveInfo) -> str: hash_ownerid = sha1(str(owner.ownerid).encode()) return hash_ownerid.hexdigest() @owner_bindable.field("orgUploadToken") @require_part_of_org -def resolve_org_upload_token(owner: Owner, info, **kwargs): +def resolve_org_upload_token( + owner: Owner, info: GraphQLResolveInfo, **kwargs: Any +) -> str: command = info.context["executor"].get_command("owner") return command.get_org_upload_token(owner) @@ -204,7 +211,9 @@ def resolve_org_upload_token(owner: Owner, info, **kwargs): @owner_bindable.field("defaultOrgUsername") @sync_to_async @require_part_of_org -def resolve_org_default_org_username(owner: Owner, info, **kwargs) -> int: +def resolve_org_default_org_username( + owner: Owner, info: GraphQLResolveInfo, **kwargs: Any +) -> Optional[str]: return None if owner.default_org is None else owner.default_org.username @@ -213,13 +222,13 @@ def resolve_org_default_org_username(owner: Owner, info, **kwargs) -> int: @convert_kwargs_to_snake_case def resolve_measurements( owner: Owner, - info, + info: GraphQLResolveInfo, interval: Interval, after: Optional[datetime] = None, before: Optional[datetime] = None, repos: Optional[List[str]] = None, is_public: Optional[bool] = None, -) -> Iterable[MeasurementSummary]: +) -> Iterable[dict]: current_owner = info.context["request"].current_owner okta_authenticated_accounts: list[int] = info.context["request"].session.get( @@ -256,7 +265,7 @@ def resolve_measurements( @owner_bindable.field("isCurrentUserActivated") @sync_to_async -def resolve_is_current_user_activated(owner: Owner, info: GraphQLResolveInfo): +def resolve_is_current_user_activated(owner: Owner, info: GraphQLResolveInfo) -> bool: current_user = info.context["request"].user if not current_user.is_authenticated: return False @@ -303,7 +312,7 @@ def resolve_is_github_rate_limited( @convert_kwargs_to_snake_case def resolve_owner_invoice( owner: Owner, - info, + info: GraphQLResolveInfo, invoice_id: str, ) -> stripe.Invoice | None: return BillingService(requesting_user=owner).get_invoice(owner, invoice_id) @@ -371,7 +380,9 @@ def resolve_ai_enabled_repos( @owner_bindable.field("uploadTokenRequired") @require_part_of_org -def resolve_upload_token_required(owner: Owner, info) -> bool | None: +def resolve_upload_token_required( + owner: Owner, info: GraphQLResolveInfo +) -> bool | None: return owner.upload_token_required_for_public_repos From 1d85bc83eb69598ed2897db49a1ee5c9168f8f37 Mon Sep 17 00:00:00 2001 From: JerrySentry Date: Thu, 17 Oct 2024 21:35:17 +0900 Subject: [PATCH 3/3] add new test and use constant --- .../owner/interactors/set_yaml_on_owner.py | 3 +- .../tests/test_set_yaml_on_owner.py | 30 +++++++++++++++++++ codecov_auth/constants.py | 1 + graphql_api/types/owner/owner.py | 3 +- 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py index 415cc9bf44..57ff1f225c 100644 --- a/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py +++ b/codecov_auth/commands/owner/interactors/set_yaml_on_owner.py @@ -13,6 +13,7 @@ ValidationError, ) from codecov.db import sync_to_async +from codecov_auth.constants import OWNER_YAML_TO_STRING_KEY from codecov_auth.helpers import current_user_part_of_org from codecov_auth.models import Owner @@ -56,6 +57,6 @@ def execute(self, username: str, yaml_input: str) -> Owner: self.authorize() self.owner.yaml = self.convert_yaml_to_dict(yaml_input) if self.owner.yaml: - self.owner.yaml["to_string"] = yaml_input + self.owner.yaml[OWNER_YAML_TO_STRING_KEY] = yaml_input self.owner.save() return self.owner diff --git a/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py b/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py index 1f47d936df..d2565ee391 100644 --- a/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py +++ b/codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py @@ -35,6 +35,16 @@ bot: foo: bar """ +good_yaml_with_comments = """ +# comment 1 +codecov: # comment 2 + + + bot: 'codecov' +# comment 3 + #comment 4 +""" + class SetYamlOnOwnerInteractorTest(TransactionTestCase): def setUp(self): @@ -113,3 +123,23 @@ async def test_yaml_syntax_error(self): str(e.value) == "Syntax error at line 3, column 13: mapping values are not allowed here" ) + + async def test_yaml_has_comments(self): + owner_updated = await self.execute( + self.current_owner, self.org.username, good_yaml_with_comments + ) + # check the interactor returns the right owner + assert owner_updated.ownerid == self.org.ownerid + assert owner_updated.yaml == { + "codecov": { + "bot": "codecov", + }, + "to_string": "\n" + "# comment 1\n" + "codecov: # comment 2\n" + "\n" + "\n" + " bot: 'codecov'\n" + "# comment 3\n" + " #comment 4\n", + } diff --git a/codecov_auth/constants.py b/codecov_auth/constants.py index a27a4f230a..a818f2a64e 100644 --- a/codecov_auth/constants.py +++ b/codecov_auth/constants.py @@ -3,3 +3,4 @@ GITLAB_BASE_URL = "https://gitlab.com" GRAVATAR_BASE_URL = "https://www.gravatar.com" AVATARIO_BASE_URL = "https://avatars.io" +OWNER_YAML_TO_STRING_KEY = "to_string" diff --git a/graphql_api/types/owner/owner.py b/graphql_api/types/owner/owner.py index aea89daadc..848ab6e8e6 100644 --- a/graphql_api/types/owner/owner.py +++ b/graphql_api/types/owner/owner.py @@ -11,6 +11,7 @@ import services.activation as activation import timeseries.helpers as timeseries_helpers from codecov.db import sync_to_async +from codecov_auth.constants import OWNER_YAML_TO_STRING_KEY from codecov_auth.helpers import current_user_part_of_org from codecov_auth.models import ( SERVICE_GITHUB, @@ -94,7 +95,7 @@ def resolve_yaml(owner: Owner, info: GraphQLResolveInfo) -> Optional[str]: current_owner = info.context["request"].current_owner if not current_user_part_of_org(current_owner, owner): return None - return owner.yaml.get("to_string", yaml.dump(owner.yaml)) + return owner.yaml.get(OWNER_YAML_TO_STRING_KEY, yaml.dump(owner.yaml)) @owner_bindable.field("plan")