Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Commit.report to GCS #10

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Conversation

giovanni-guidini
Copy link
Contributor

This is the API changes that will move Commit.report to GCS. Missing tests on the model.

Purpose/Motivation

What is the feature? Why is this being done?

Links to relevant tickets

What does this PR do?

Include a brief description of the changes in this PR. Bullet points are your friend.

Notes to Reviewer

Anything to note to the team? Any tips on how to review, or where to start?

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.

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #10 (bb10593) into main (40f152f) will decrease coverage by 0.05%.
The diff coverage is 88.63%.

❗ Current head bb10593 differs from pull request most recent head 53c5311. Consider uploading reports for the commit 53c5311 to get more accurate results

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

@@           Coverage Diff           @@
##            main     #10     +/-   ##
=======================================
- Coverage   95.23   95.18   -0.05     
=======================================
  Files        559     554      -5     
  Lines      14030   14004     -26     
=======================================
- Hits       13361   13329     -32     
- Misses       669     675      +6     
Flag Coverage Δ
unit 95.17% <88.63%> (-0.06%) ⬇️
unit-latest-uploader 95.17% <88.63%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/chart/views.py 86.04% <ø> (ø)
api/internal/repo/serializers.py 100.00% <ø> (ø)
api/public/v2/branch/serializers.py 100.00% <ø> (ø)
api/shared/commit/mixins.py 100.00% <ø> (ø)
compare/admin.py 100.00% <ø> (ø)
graphql_api/dataloader/commit.py 100.00% <ø> (ø)
reports/models.py 96.22% <50.00%> (+3.56%) ⬆️
core/models.py 97.43% <73.33%> (-1.66%) ⬇️
...it_report_rename_report_commit__report_and_more.py 100.00% <100.00%> (ø)
graphql_api/actions/commits.py 100.00% <100.00%> (ø)
... and 4 more

... and 39 files with indirect coverage changes

@giovanni-guidini giovanni-guidini force-pushed the gio/migrate-commits-report-to-gcs branch from e8f3d33 to e744a2a Compare July 19, 2023 08:28
@giovanni-guidini giovanni-guidini marked this pull request as ready for review July 19, 2023 08:39
This is the API changes that will move `Commit.report` to GCS.
- Alter `report` field to be `_report` (the db_field) and `_report_storage_path` (the archive_field)
- Create the associated migration
- Turn `report` into an `ArchiveField`
- Adjust tests

There were some minor tweaks to `ArchiveField`, and I had to drop the typehint in `get_repository` to avoid circular dependencies
I also decided to move the config lookup of `should_write_data_to_storage` to a shared function, given they will be similar for all fields. Notice we can still turn individual fields on/off independently, but the "only_codecov" and "repo_ids" are shared now.

Every field still has their own `should_write_data_to_storage` to account for differences in the objects, and allow for individual behavior. Also less duplicated code, yay

Something else I had to do is include an `encoder` in the `ArchiveField` so we can continue using the same encoder for the db_field and the archive_field
Copy link
Contributor

@scott-codecov scott-codecov left a comment

Choose a reason for hiding this comment

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

Looks good! Just 1 thing related to the config values

return master_write_switch and (
is_codecov_repo or is_in_allowed_repos or not only_codecov
return should_write_data_to_storage_config_check(
master_switch_key="report_details_files_array",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a new config key for commits.report?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should, yes. I used "commit_report" as the master key.
(this file is the ReportDetails one btw)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh oh, I see 👍

@giovanni-guidini giovanni-guidini merged commit 0c219cb into main Jul 21, 2023
5 of 6 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/migrate-commits-report-to-gcs branch July 21, 2023 14:21
scott-codecov added a commit that referenced this pull request Jul 21, 2023
* main:
  Add api_health endpoint
  Move `Commit.report` to GCS (#10)
  fix: Set SameSite=None; Secure for sessionid cookie
  delete unnecessary redacted secrets from config
  Get rid of reduntant validation error
  Update shared. (#20)
  fix: Handle no current owner in GraphQL resolvers
  Return and catch validation error
  Addressed feedback
  Add mutation to start trial
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants