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

ref: Remodel graphql schemas #742

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

ref: Remodel graphql schemas #742

wants to merge 5 commits into from

Conversation

suejung-sentry
Copy link
Contributor

@suejung-sentry suejung-sentry commented Aug 9, 2024

This is a draft PR that serves as a way to view the proposed diff for Restructure GraphQL project - https://www.notion.so/sentry/Project-Plan-Restructure-GraphQL-9b47b646b797451090c733be880615e7?pvs=4

Please comment any suggestions!!

coverageSha: String
hits: Int
misses: Int
lines: Int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shown as deleted in this diff for illustrative purposes. In practice, we plan to delete the old fields only after everything is migrated over to the new fields and stable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will you be duplicating the resolvers during the migration process as well? Ideally there would be a way to bind both the new and old schemas to the same resolvers at the same time so that the Python codes don't get duped.

branch: String
): [Measurement!]!

### FLAGS ###
Copy link
Contributor Author

@suejung-sentry suejung-sentry Aug 13, 2024

Choose a reason for hiding this comment

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

We have the option of keeping Flags and Components as flat fields within Coverage as here, or pulling them out into separate sub types (like below). That would futureproof it getting added to TestAnalysis or BundleAnalysis as filters. But we thought that would be overengineering for now.

type FlagMeasurements {
   flagsCount: Int!
   flagsMeasurementsActive: Boolean!
   flagsMeasurementsBackfilled: Boolean!
...
}

Another option is

Measurements {
  count: Int!
  active: Boolean!
  backfilled: Boolean!
  ...
}
type CoverageV2 {
   flagMeasurements: Measurements
   componentMeasurements: Measurements
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The schema under "Another option is" looks so clean! But yeah right now there's some small nuances between different types of measurements (eg coverage, flag, component, bundle analysis) that will require alot of refactoring to make it happen :(

misses: Int
lines: Int
coverageSha: String
percentCovered: Float # formerly repo.coverage
Copy link
Contributor Author

@suejung-sentry suejung-sentry Aug 13, 2024

Choose a reason for hiding this comment

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

We have the option of using the CoverageTotals type instead of these flat fields hits, mises, etc.

We decided against that because the query pattern may be different, and data may be coming from a different table. Open for discussion.

@@ -25,6 +36,7 @@ type Repository {
last: Int
before: String
): PullConnection @cost(complexity: 10, multipliers: ["first", "last"])
# TODO - does this refer to commits on main only?

Choose a reason for hiding this comment

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

If anyone can answer this question, please let us know. What happens for other branches?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just on default branch I'm pretty sure. You can get any commit by its SHA with Repository { commit(id: SHA) {} } per the API docs. idk how that maps onto this file, however lol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Repository.commits can be any set of commits depending on the filters used (see: filters: CommitsSetFilters)

In the filters it specifies:

type CommitsSetFilters {
  hideFailedCI: Boolean
  branchName: String
  pullId: Int
  search: String
  states: [CommitState!]
  coverageStatus: [CommitStatus!]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Dangg that's good to know thanks for clarifying @JerrySentry

@@ -1,15 +1,26 @@
# Repository is a named collection of files uploaded
Copy link
Contributor

Choose a reason for hiding this comment

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

btw if you follow this notation:
https://spec.graphql.org/June2018/#sec-Descriptions
these doc strings would appear in the ariadne gql playground tool.

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.

4 participants