Skip to content
This repository was archived by the owner on Nov 11, 2019. It is now read-only.

Conversation

@La0
Copy link
Contributor

@La0 La0 commented May 16, 2019

This PR replace the /v2/path endpoint to use GCP covdir data + a redis cache.

Organisational data is stored in redis:

  • a list of changesets with their pushes (eg: changeset:deadbeef ➡️ 12345)
  • a sorted set of the GCP reports found, sorted by their push id (eg: reports ➡️ {12345: "deadbeef", ...}

The coverage data is downloaded from GCP, extracted and stored in a local file path.
⚠️ No coverage data is available in Redis

@La0 La0 self-assigned this May 16, 2019
@La0 La0 force-pushed the ccov-path-gcp branch 3 times, most recently from c740ef6 to 63db25e Compare May 21, 2019 09:43
@La0 La0 requested a review from marco-c May 21, 2019 09:43
@La0 La0 marked this pull request as ready for review May 21, 2019 09:43
@La0 La0 requested a review from garbas as a code owner May 21, 2019 09:43
@La0 La0 force-pushed the ccov-path-gcp branch 2 times, most recently from 1400710 to 4808081 Compare May 21, 2019 09:50
raise Exception('Path {} not found in report'.format(object_path))
report = report['children'][part]

def _clean_object(obj, base_path, depth=0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if some of the changes here couldn't be avoided, leaving the onus on the frontend.
E.g. adding the type could be unnecessary, as the frontend can check if there are 'children' itself.
Similarly for path.

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 did that to ease the work on the frontend side, but i'm totally ok with leaving the covdir format as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah let's do that. At least removing type and path. This way we keep the backend as lean as possible, almost just serving the report file.

def coverage_for_path(path='', changeset=None):
def coverage_latest(repository=DEFAULT_REPOSITORY):
'''
List the last 10 reports available on the server
Copy link
Collaborator

Choose a reason for hiding this comment

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

The frontend will likely only need the latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now... I did that because it was easy to do now, and could provide a quick <select> on the frontend to display recent variations of a file's coverage

@La0 La0 force-pushed the ccov-path-gcp branch from 986bbb2 to 2be5f76 Compare May 21, 2019 17:21
logger.info('Reports will be stored in {}'.format(self.reports_dir))

# Load most recent reports in cache
for repo in ('mozilla-central', ):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: define repositories in a constant at the beginning of the file

if not chunk:
break
output.write(chunk)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove the zstd file now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as we are on heroku yes.
When we switch to GCP, we will have some FS persistence, and could retain archives as long term storage (and only keep the warm json extracted)

raise Exception('No report found')
return results[0]

def find_closest_report(self, repository, revision):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: we might want to keep a consistent naming and either always use changeset or revision

Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

LGTM! Some modifications will be required for getting filters (#1227) to work, but they should be reasonable.

@La0 La0 force-pushed the ccov-path-gcp branch from 34b9840 to 380a607 Compare May 22, 2019 10:25
@La0 La0 force-pushed the ccov-path-gcp branch from 380a607 to 95e4462 Compare May 22, 2019 10:26
@La0 La0 merged commit 1259c2b into mozilla:master May 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants