Skip to content
This repository was archived by the owner on Jun 13, 2025. It is now read-only.

Add support for saving comments in global YAML file #895

Merged
merged 7 commits into from
Oct 18, 2024
Merged

Add support for saving comments in global YAML file #895

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f6aa70c
add support for comments in global yaml
JerrySentry Oct 16, 2024
b29155b
Merge branch 'main' into oct_15_comment
JerrySentry Oct 16, 2024
ba7e350
add typing 1
JerrySentry Oct 16, 2024
c920eac
erge branch 'main' into oct_15_comment
JerrySentry Oct 16, 2024
4187f39
Merge branch 'main' into oct_15_comment
JerrySentry Oct 17, 2024
1d85bc8
add new test and use constant
JerrySentry Oct 17, 2024
909233d
Merge branch 'main' into oct_15_comment
JerrySentry Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions codecov_auth/commands/owner/interactors/set_yaml_on_owner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import html
from typing import Optional

import yaml
from shared.validation.exceptions import InvalidYamlException
Expand All @@ -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)
Expand All @@ -49,10 +50,12 @@ def convert_yaml_to_dict(self, yaml_input):
raise ValidationError(message)
Copy link
Contributor

@suejung-sentry suejung-sentry Oct 16, 2024

Choose a reason for hiding this comment

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

I'd guess in a perfect world we would want validate_yaml (in shared) to reject if any reserved keys (like to_string) are in it. It's probably a lot of work for something unlikely, though so can let it be

Copy link
Contributor

@suejung-sentry suejung-sentry Oct 16, 2024

Choose a reason for hiding this comment

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

Oh never mind I had missed the note in the PR description! Sounds like that will get covered later - nice!


@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()
self.owner.yaml = self.convert_yaml_to_dict(yaml_input)
Copy link
Contributor Author

@JerrySentry JerrySentry Oct 16, 2024

Choose a reason for hiding this comment

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

One of two of the actual changes.

if self.owner.yaml:
self.owner.yaml["to_string"] = yaml_input
Copy link
Contributor

@suejung-sentry suejung-sentry Oct 16, 2024

Choose a reason for hiding this comment

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

We can consider pulling this string key out into a constant so it's more discoverable (not sure all the Python conventions for this)

self.owner.save()
return self.owner
14 changes: 12 additions & 2 deletions codecov_auth/commands/owner/interactors/tests/test_set_yaml_on_owner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,25 @@ 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",
Copy link
Contributor

@suejung-sentry suejung-sentry Oct 16, 2024

Choose a reason for hiding this comment

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

Can we add an example after line 36 in this file where the to_string would have comments? Like below I guess

good_yaml_with_comments = """
codecov:
  # this is a yaml comment
  require_ci_to_pass: yes
"""

'\ncodecov:\n  # this is a yaml comment\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(
self.current_owner, self.org.username, good_yaml_with_quotes
)
# 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, "")
Expand Down
59 changes: 35 additions & 24 deletions graphql_api/types/owner/owner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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,
)
Expand All @@ -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")
Expand All @@ -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, []
Expand All @@ -81,19 +82,19 @@ 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):
Copy link
Contributor Author

@JerrySentry JerrySentry Oct 16, 2024

Choose a reason for hiding this comment

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

The other change.

return
return yaml.dump(owner.yaml)
return None
return owner.yaml.get("to_string", yaml.dump(owner.yaml))


@owner_bindable.field("plan")
Expand Down Expand Up @@ -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, []
Expand Down Expand Up @@ -174,37 +177,43 @@ 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)


@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)


@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


Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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


Expand Down
12 changes: 12 additions & 0 deletions services/tests/test_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@suejung-sentry suejung-sentry Oct 16, 2024

Choose a reason for hiding this comment

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

In case you didn't see this, I wanted to see what final_commit_yaml does - there is this essay about the logic 😂
https://github.com/codecov/shared/blob/f40874907d3ddbe6993c8016c3f09306cc14a9f8/shared/yaml/user_yaml.py#L128

I think it's not a problem because your change only deals with the "ownerYaml".

I also checked what other clients than the graphql resolver for the owner yaml in this PR may be accessing the revised owner.yaml field. Searching "owner.yaml" shows nothing in shared and a couple spots in worker that all point back to the same final_commit_yaml function above. That function returns a dictionary that doesn't appear to matter if it has the extra to_string on it from what I can tell. So no additional work there needed from what I can tell.

assert config.get("to_string") is None
assert "to_string" not in config.to_dict()
Loading