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

build: change upload-to-s3 vars to upload-to-storage #34105

Merged
merged 2 commits into from May 9, 2022

Conversation

VerteDinde
Copy link
Member

@VerteDinde VerteDinde commented May 5, 2022

Description of Change

A follow up to #34104, this PR changes all instances of upload-to-s3 to upload-to-storage. We now use Azure, and this variable is now platform agnostic, should we want to switch platforms in the future.

Please do not merge until the first PR is merged, and all instances of this variable have been confirmed changed in CircleCI and AppVeyor.
This PR is ready for review. I checked both Appveyor and CircleCI, and neither appears to be storing a default value for UPLOAD_TO_S3. Since both already contain the necessary info for Azure, I think this should "just work".

Checklist

Release Notes

Notes: no-notes

@VerteDinde VerteDinde changed the title build: change upload-to-s3 vars to upload-to-az build: change upload-to-s3 vars to upload-to-storage May 5, 2022
Base automatically changed from remove-s3-usage to main May 6, 2022 04:40
@VerteDinde VerteDinde force-pushed the upload-to-az-variable-change branch from b304471 to 1accc24 Compare May 9, 2022 03:31
@VerteDinde VerteDinde marked this pull request as ready for review May 9, 2022 03:32
@VerteDinde VerteDinde requested a review from a team as a code owner May 9, 2022 03:32
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

overall this PR is fine but @MarshallOfSound discovered an issue where usingupload-to-s3 causes overwriting the headers tarballs of legit releases. @MarshallOfSound's suggestion was that this flag should upload to a different container to prevent contention.

Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

disregard that last comment. However in a future PR we should update upload-to-storage to exercise all upload paths (right now it skips some) and upload to an alternate container. But that is not a blocker for this PR.

@jkleinsc jkleinsc merged commit 6fea352 into main May 9, 2022
@jkleinsc jkleinsc deleted the upload-to-az-variable-change branch May 9, 2022 13:34
@release-clerk
Copy link

release-clerk bot commented May 9, 2022

No Release Notes

@trop
Copy link
Contributor

trop bot commented May 9, 2022

I have automatically backported this PR to "15-x-y", please check out #34142

@trop
Copy link
Contributor

trop bot commented May 9, 2022

I have automatically backported this PR to "19-x-y", please check out #34143

@trop
Copy link
Contributor

trop bot commented May 9, 2022

I have automatically backported this PR to "17-x-y", please check out #34144

@trop
Copy link
Contributor

trop bot commented May 9, 2022

I have automatically backported this PR to "16-x-y", please check out #34145

@trop
Copy link
Contributor

trop bot commented May 9, 2022

I have automatically backported this PR to "18-x-y", please check out #34146

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* build: change upload-to-s3 vars to upload-to-az

* build: change upload-to-az to upload-to-storage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants