-
Notifications
You must be signed in to change notification settings - Fork 33
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
Download specs gzipped for 'compute vocabulary' and visitor list operations. #1122
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1122 +/- ##
==========================================
- Coverage 69.95% 69.92% -0.03%
==========================================
Files 145 145
Lines 12022 12040 +18
==========================================
+ Hits 8410 8419 +9
- Misses 2884 2890 +6
- Partials 728 731 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -91,7 +97,7 @@ func TestFetch(t *testing.T) { | |||
if err != nil { | |||
t.Fatalf("Failed to fetch spec contents: %s", err) | |||
} | |||
if string(spec.Contents) != specContents { | |||
if !bytes.Equal(spec.Contents, gzippedSpecContents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following two changes show an inconsistency -- ListSpecs
is returning specs uncompressed, even when they are downloaded compressed (as of this PR). GetSpecs
was already downloading specs compressed but returning them without uncompressing. I think it's probably better for GetSpecs
to also uncompress specs before returning them, but think it might overcomplicate this PR by making that change here.
@@ -164,7 +164,7 @@ func (task *computeVocabularyTask) Run(ctx context.Context) error { | |||
return nil | |||
} | |||
} else { | |||
return fmt.Errorf("we don't know how to summarize %s", task.specName) | |||
return fmt.Errorf("we don't know how to compute the vocabulary of %s", task.specName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just a kick of a tire to get the CLA check to run again... but it was also needed
I noticed that
registry compute vocabulary
was fetching specs without compression (causing failures for specs that were larger than 4MB uncompressed #1110). When fixing this, I scanned the code for other calls toGetApiSpecContents
that weren't accepting gzipped specs and found two inpkg/visitor
. Because these functions were returning spec contents uncompressed, I also added code to uncompress gzipped specs before returning them. Looking ahead, we might want to more tightly encapsulate the calls toGetApiSpecContents
so that we download gzipped specs as often as possible.