-
Notifications
You must be signed in to change notification settings - Fork 84
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
[ENG-10412] Upload projectMetadata file #2149
[ENG-10412] Upload projectMetadata file #2149
Conversation
5306103
to
621303b
Compare
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.
It looks reasonable, I just have few more questions before merging it
621303b
to
56b5c8b
Compare
56b5c8b
to
6ec3bfc
Compare
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.
It might be good to add deployment order to each PR description, so it is clear in which order the changes should be deployed.
Last few comments:
7995c38
to
05354c6
Compare
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.
I think that it looks good at the moment. Nice! 👏
Thanks for applying my suggestions 🙇
My final suggestion is to specify a deployment order, so it is clear what should land when
83fe69c
to
3af7343
Compare
3af7343
to
588127a
Compare
Size Change: +578 B (0%) Total Size: 51.4 MB
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2149 +/- ##
==========================================
- Coverage 53.78% 53.67% -0.11%
==========================================
Files 525 525
Lines 19115 19158 +43
Branches 4037 4043 +6
==========================================
+ Hits 10279 10281 +2
- Misses 8114 8151 +37
- Partials 722 726 +4 ☔ View full report in Codecov by Sentry. |
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.
Looks better. A few comments!
71b75d4
to
87ccf9b
Compare
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.
Few minor comments, code looks reasonable to me 👍 If the manual testing works fine after merging WWW it should be good to go 👏
Please rebase/merge main into this PR
419a03c
to
bc2ef82
Compare
); | ||
startTimer(timerLabel); | ||
|
||
const metadataLocation = path.join(getTmpDirectory(), `${uuidv4()}-eas-build-metadata.json`); |
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.
Is the filename preserved — do GCS files have the same name? Maybe it would be better to call them eas-build-metadata-${uuidv4()}.json
if that's the case?
return bucketKey; | ||
const { metadataLocation } = await uploadMetadataFileAsync<TPlatform>(projectTarball, ctx); | ||
if (metadataLocation) { | ||
return { bucketKey, metadataLocation }; |
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.
I remain on the position the fact we have different names (key, location) for the same thing in the same object and the newer name is of less specific meaning is wrong. This comment has been left unanswered for a couple iterations now (first mentioned here) and the conversation marked as resolved twice which is confusing for me.
09cbb45
to
6f3b4f3
Compare
6f3b4f3
to
7a40db5
Compare
✅ Thank you for adding the changelog entry! |
Why
ENG-10412 We want to allow user to easily check the content of build archive. This should streamline debugging for certain cases.
How
CLI generates file containing a list of files present in project archive. This file is uploaded together with project archive to the same bucket.
The reason for generating this file at this step is that:
Uploading the file to GCP (as opposed to, for example, storing it in the DB) has following advantages:
Test Plan
Create new build.
Deploy plan