Make CI work without needing an Amazon S3 or PackageCloud.io account

[DeeDeeG]

Summary

There should be a simple and easy way for uploads to Amazon S3 and PackageCloud.io to be disabled in CI. This allows CI to pass without paying for paid services just to hack on Atom in a fork.

Motivation

@aminya and I have been working on a fork of Atom. When running the CI, we have noticed that the "Atom Production Branches" and "Atom Nightly" pipelines require authentication to a paid Amazon S3 account and a paid PackageCloud.io account.

Having a good way for CI to run without attempting to upload to these services is friendly to forks. We could roll our own solution to this at our fork, but any other fork that might come along after us would have to reinvent that wheel, so it would be amazing to centralize the approach here at upstream.

When editing the CI files, we are still hoping to upstream our changes. But this is something I would expect upstream to care fairly strongly about, since it is part of the official release process for stable/official Atom. Rather than try to predict what upstream wants, it seems prudent to ask about it and start a discussion.

Describe alternatives you've considered

We could disable uploading to PackageCloud.io by editing the flags in the pipeline .yml configs, used to call script/vsts/upload-artifacts.js. But these changes would not be suitable for upstreaming. And this could be prone to merge conflicts with upstream if you ever need to update the pipeline .yml files.

This also does not disable uploads to Amazon S3, so editing script/vsts/upload-artifacts.js is required in any case.

In order to avoid merge conflicts and the productivity loss of fragmented and forked CI configs, we are hoping to avoid carrying patches for CI for ourselves.

Additional context

We have been working on an upstream-friendly implementation of these changes here: atom-community#66

This WIP implementation is designed to require no changes (can be merged as-is, "just works") by upstream, while offering forks an easy way to toggle off uploads to paid services (S3 and PackageCloud.io).

With the current WIP design, forks would just need to set a variable (in the Azure DevOps UI) SKIP_UPLOADING_TO = e.g. s3,packagecloud to turn any given service off.

[Arcanemagus]

I can see how this would benefit a fork, but what benefit does this provide to the main Atom repo? Adding the complexity of making this able to be toggled is more code to maintain here for no benefit. The entire point of a fork is that you are going in a different direction from the main project so you will need to be changing quite a bit of the code.

[DeeDeeG]

I can see how this would benefit a fork, but what benefit does this provide to the main Atom repo?

I'll admit: In a vacuum, this provides nothing immediately to upstream. But for some projects, contributions from the community and [personal, org] forks make up a large portion of new changes. So it could be meaningful to Atom to have a robust community of forks contributing code that has already been CI-tested before even being posted as PRs. And it affords the community a bit more independence in how they operate. IMO it is an ecosystem investment, rather than strictly a benefit to upstream.

---

Another thing: I was motivated to post this issue because the CI config is somewhat modular already, allowing uploads to PackageCloud be "turned on or off" with flags, but depending on your perspective (we have a fork's perspective obviously):

• It is deficient at disabling S3, as uploads to S3 always happen regardless
• Enabling/disabling things by hard-coded flags is prone to merge conflicts
• The flags used to be env variables IIRC, but was changed to yargs and flags at some point, making this feature harder or at least more redundant for us to code downstream.

I'm motivated to post here because the existing solution is "so close, but not quite there", and our WIP solution would add a second layer of configurability, which just doesn't seem right. More complexity than there probably should be. Discussing with upstream in the open puts more options on the table to rewrite this in a way that makes sense for upstream and forks.

In short: The best design here (for us, and in the long view, for a robust fork-friendly ecosystem) is only possible through coordinating with upstream and agreeing on a design.

Worst-case scenario is we have to carry patches for the CI. Best-case scenario, upstream is interested, we improve the CI code, and everyone benefits from the well-considered code.

P.S. it is a default assumption that we make that upstream may not be interested. That's fine. But if that mindset prevents us from ever reaching out like this and making suggestions that's probably going too far in that direction. We will take any answer on this, even if it's "no" or "no reply." We're just reaching out in good faith to open the dialog on this, and if we get a reply, great. Thanks for your reply already.

---

P.P.S.

The entire point of a fork is that you are going in a different direction from the main project so you will need to be changing quite a bit of the code.

Kind of. My motivation for participating in the fork is to move faster on some backlog items, like the Electron upgrades. And to prove out the upgrade path so upstream can move faster with what might appear from an outsider's perspective to be limited engineering resources. Or tracking down and squashing blocking bugs for same. We want to upstream some things of that nature, or contribute to those things being easier to move forward on. As such, being close with upstream is an express goal of our fork. In my personal view, "going our own way" is a fallback option, a "plan B", not a first choice or a "plan A."

[DeeDeeG]

Thanks for the reply earlier. We found a way to handle this automatically.

We are able to detect if the vars are unset in Azure Pipelines (despite that Azure Pipelines makes them non-empty from a JS point of view, see below). If credentials/API keys are unset on the Pipelines side of things, then we skip uploading to whichever service(s) the credentials vars weren't set for.

https://github.com/atom-ide-community/atom/pull/66/files

---

What was tripping me up: We can't check if these vars are "empty" or "unset" in JS, because they will either be some manually set value, or else when unset in Azure Pipelines, the env vars will hold literally $(VAR_NAME_HERE). Dollar sign and parentheses and ALL_CAPS_VAR_NAME all will be in the env var.

See the chart in this section of the docs: https://docs.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch#runtime-expression-syntax

Solution: rather than simply checking for an empty/falsy env var in JS, we can check that the var doesn't equal the default "unset" version of itself per DevOps, e.g. process.env.PACKAGE_CLOUD_API_KEY !== '$(PACKAGE_CLOUD_API_KEY)'.

To be extra safe, we can additionally check that the var is JavaScript truthy. This is to handle the scenarios when someone has set the vars to be the empty string, or some refactor some time in the future allows the vars to be JS undefined.

At that point we're back to basically reading env vars, and there's no fancy rewrite of the code needed. No new variables are needed, either.

---

We might post this as a PR after we've been using it for a bit. It's working great so far. The idea being for the repo to be ready to be forked by anybody and have working CI right away.

[aminya]

@DeeDeeG I don't think that extra check is needed. The process.env variable is either empty or it has some value. We don't need to use Azure's syntax here.

[DeeDeeG]

Closing here.

This is 99% solved, so it's no-longer an "actionable"/open issue IMO.

(Still working out the fine details and waiting for a response from Azure pipelines developers on a question we had before we make a final version of the code.)

If we post a PR here, we can still point to this issue even if it's "closed."