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

Use a simpler condition to determine whether to publish crash reports #19180

Merged
merged 1 commit into from Apr 18, 2019

Conversation

Projects
None yet
2 participants
@nathansobo
Copy link
Contributor

commented Apr 18, 2019

Immediately after merging #19171, I had second thoughts about the condition I was using to decide whether or not to upload crash reports. I was using the isReleaseBranch variable being false as my indicator, but then I started wondering if that might be false in situations where we are still exposing secrets via environment variables.

This PR changes the condition to test for the presence of secrets directly.

Tasks:

  • Ensure crash reports upload based on the new condition.
  • Ensure crash reports don't upload on builds that expose our secrets, even if there are crashes.

@nathansobo nathansobo requested a review from smashwilson Apr 18, 2019

@nathansobo nathansobo force-pushed the ns/tighten-crash-upload-condition branch from 0a10d62 to 364961d Apr 18, 2019

Only upload Windows crash reports if the S3 key environment var is null
This seems like a more robust test than the isRelease variable I was
testing previously. Our goal is to not leak secrets such as the S3 key,
so this is a more direct test of that.

@nathansobo nathansobo force-pushed the ns/tighten-crash-upload-condition branch from ceaaf4c to 7ef8f06 Apr 18, 2019

@nathansobo nathansobo merged commit cbd4641 into master Apr 18, 2019

2 checks passed

Atom Pull Requests #20190418.20 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the ns/tighten-crash-upload-condition branch Apr 18, 2019

@@ -114,7 +114,7 @@ jobs:
PathtoPublish: $(Build.ArtifactStagingDirectory)/crash-reports
ArtifactName: crash-reports
displayName: Publish crash reports on non-release branch
condition: and(failed(), eq(variables['IsReleaseBranch'], 'false'))
condition: and(failed(), eq(variables['ATOM_RELEASES_S3_KEY'], ''))

This comment has been minimized.

Copy link
@smashwilson

smashwilson Apr 19, 2019

Member

Hah! Oh, right, I just commented on this on the other PR 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.