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

Fix "Upload Artifacts" job in CI (makes CI pass) #471

Merged
merged 5 commits into from
Sep 19, 2022

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 18, 2022

Requirements for Contributing a Bug Fix (from template, click to expand):

Identify the Bug

CI isn't passing, because it tries to upload binary blobs to Azure Storage, but without the needed credentials. This errors out, causing CI to fail (even after the build and tests may have succeeded.)

Description of the Change

  • Update script/vsts/upload-artifacts.js to skip uploading binary blobs to Azure Storage if env var ATOM_RELEASES_AZURE_CONN_STRING is unset or empty (more precisely: if it is falsy in JS).
  • Update the "Upload CI Artifacts" job in the CI YAML files so that it uses the intermediate variables workaround, which makes sure env vars will be seen as undefined/empty in scripts.
    • This is restoring a part of the solution we had before (from CI: option to skip upload #99), but I think it got reverted during a merge of upstream commits at some point?
  • Use unshortened URL in the comment explaining the workaround
    • (This is an optional change, not super important. Just my personal preference.)

Alternate Designs

None.

Possible Drawbacks

None.

Verification Process

CI should pass if this PR is done right.

NOTE: The PR pipeline will not test this change. I will manually run the release branch pipeline on my personal CI instance to see if it passes. CI passing directly on this PR will be the "Pull Requests" pipeline, which has nothing to do with this change.

Update: It works for me on my personal CI instance: CI link

Environment variable "process.env.ATOM_RELEASES_AZURE_CONN_STRING," is not set, skipping Azure upload.

Release Notes

N/A

Upstream switched from S3 to Azure Storage for uploading binary blobs
during CI. (Used for serving releases from the official website?)

Just like we used to have it set up to skip uploading to Amazon S3
(due to this fork's lack of credentials), this commit adjusts
to the changes from upstream to also skip uploading to Azure.

(This fork doesn't have credentials for an Azure Storage account,
so skipping the upload is needed in order for CI to pass.)

(This first commit only adjusts the JS script that does the uploading.
Another change is needed to pass the env vars properly to the script.)
Update the release-branch-build and nightly-release pipeleine YAML to:

- Add the _VAR_NAME_HERE intermediate var for Azure
- Delete unused Amazon S3-related vars
- Restore use of the intermediate vars (These make sure that vars are
  properly seen as empty/undefined in scripts, if unset in CI. They
  were accidentally reverted during a merge of upstream commits, I
  think?)

This makes sure the upload-artifacts.js script is able to properly
detect whether the env vars are unset, so it can skip attempting to
upload stuff to services we can't auth to (due to no credentials).

Skipping these uploads avoids errors, and thus allows CI to pass.
URL shorteners are kind of iffy IMO. Makes link hijacking more likely.
Better to use a direct link, IMO.
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 19, 2022

It worked on my personal CI instance:

Passing CI on Release Branch Build pipeline on my CI: link to passing CI run

See this line specifically, from the output of my CI run's "Upload Artifacts" job:

Environment variable "process.env.ATOM_RELEASES_AZURE_CONN_STRING," is not set, skipping Azure upload.

There is a tiny typo -- I included an unnecessary comma in the console.log() string when skipping upload to Azure... Can fix that real quick if wanted. Edit: I pushed a commit to delete the extra comma.

@aminya aminya merged commit eb8f6bf into atom-community:master Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants