Add support for saving comments in global YAML file #895
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
=======================================
Coverage 96.31% 96.31%
=======================================
Files 823 823
Lines 19092 19098 +6
=======================================
+ Hits 18389 18395 +6
Misses 703 703
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -49,10 +50,12 @@ 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() | |||
| self.owner.yaml = self.convert_yaml_to_dict(yaml_input) | |||
There was a problem hiding this comment.
One of two of the actual changes.
| 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): |
There was a problem hiding this comment.
The other change.
There was a problem hiding this comment.
Nice! I like how simple it is!
Probably worth end to end testing in staging to confirm as much as possible, as I could see some of the parsing ending up weird, or any edge cases around escaping. Also not sure how to test that the other clients than the graphql resolvers are happy with the change than smoke testing user flows in staging
| self.validate() | ||
| 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 |
There was a problem hiding this comment.
We can consider pulling this string key out into a constant so it's more discoverable (not sure all the Python conventions for this)
| "codecov": { | ||
| "require_ci_to_pass": True, | ||
| }, | ||
| "to_string": "\n" "codecov:\n" " require_ci_to_pass: yes\n", |
There was a problem hiding this comment.
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'
| require_ci_to_pass: no | ||
| to_string: hello | ||
| """ | ||
| config = yaml.final_commit_yaml(self.commit, self.org) |
There was a problem hiding this comment.
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.
| @@ -49,10 +50,12 @@ def convert_yaml_to_dict(self, yaml_input): | |||
| raise ValidationError(message) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oh never mind I had missed the note in the PR description! Sounds like that will get covered later - nice!
Add a key to the yaml JSON field to store the comments included YAML str in its original form.
Things considered/validated:
UserYamlobject, values are fetched by specific keysCodecovYamlValidator, when JSON is stored into the column it is already validated, however will add a followup PR in shared to future proof to protect the key from being used in the codecov YAML schema.Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.