Conversation
These env vars are credentials to authenticate to S3. If they're unset, we can't possibly upload to S3. So we shouldn't try to.
We can't authenticate to PackageCloud without an API key, so don't try to.
The bucket is optional, but Atom uses it. So we will check for this too, just in case https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html
So when they are not available they become an empty string https://docs.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch#runtime-expression-syntax 
This reverts commit b5d6cb9.
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
Workaround for Azure issue: https://bit.ly/2CK8itc
DeeDeeG
reviewed
Aug 21, 2020
Member
There was a problem hiding this comment.
Obviously this design is the one we've settled on. 👍
I have some low-stakes comments for my preferences for this.
Variable syntax and names (yml):
- I think this syntax is nicer:
$[variables.SOME_VAR](rather than$[ variables['SOME_VAR'] ]) - I like making the intermediate var name significantly different, so when you glance at the file you can easily notice they are different. Also helps with using Cltrl + F to find only the intermediate var or only the real var.
- I was planning on using
s3-key,s3-secretandpackage-cloud-key.- To me, lowercase indicates that we are able to use this within the
yml, but not scripts (at least not before the additional step of passing viaenv:block)
- To me, lowercase indicates that we are able to use this within the
- I was planning on using
Not as important, but I like a clean commit history:
- This can be reduced to 2-4 commits.
- Change the JavaScript to know to skip uploading
- Can be one commit, or separate commits for
upload-artifactsandupload-crash-reports
- Can be one commit, or separate commits for
- Update the .yml
- can be one commit, or separate commits for
nighty-releaseandrelease-branch-build
- can be one commit, or separate commits for
- Change the JavaScript to know to skip uploading
Member
Author
Using |
Member
|
Commenting for the historical record: We went with this PR as a re-vamped version of #66. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue or RFC Endorsed by Atom's Maintainers
#1 (comment)
Description of the Change
script/vsts/upload-artifacts.jsscript/vsts/upload-crash-reports.js.deb,.rpm) to PackageCloud.io if API key isn't set, inscript/vsts/upload-artifacts.jsThis is to stop these uploads from being attempted with misconfigured/blank credentials or upload destinations. Such misconfigured uploads error out and cause our CI to fail at the last step, after all tests have passed.
Alternate Designs
None.
Possible Drawbacks
None
Verification Process
CI is showing that these changes are working. See this comment for details: #66 (comment)
Release Notes
N/A