[BUGFIX] Fix support for non-master default branch in packs

[cognifloyd]

• Fix support for non-master default pack branches

  If any packs end up using main instead of master as their default branch, then deploying the index will fail without this change.

  Currently only the test and test2 packs use an non-master default branch (aaa). So, the impact of this change is minimal.

• Provide PAT error message as early as possible

  Resolves #109 by printing a helpful error message much sooner if a PAT has expired.

• Add DO_NOT_ADD_TO_EXCHANGE_INDEX for CI packs

  This variable should be set to true (a string, not a boolean) in CircleCI's web UI for each test pack. This way, such test packs can be used to test everything in the CI workflows including changes to the deploy step's deployment script.

[cognifloyd]

I added SKIP_INDEX to address preventing a pack from being added to the index.

See StackStorm-Exchange/stackstorm-test#16 and StackStorm-Exchange/stackstorm-test2#8

[arm4b]

Left comments in both PRs.

If we never use non-master branches in Exchange repos for deployment, - this functionality is not needed. Would be great to avoid overloading the scripts and stick to previous easiest solutions in those repos.

[cognifloyd]

We should probably also remove python3_test from the exchange and set it to SKIP_INDEX as well.

[cognifloyd]

I prefer explicit support in the deployment script versus an obscure, undocumented hack in the CircleCI config.

[cognifloyd]

As far as master vs other branch names, we should make sure that pack maintainers can choose a different branch name: https://github.com/github/renaming

[blag]

I agree somewhat with Eugen. We can pull the SKIP_INDEX from an environment variable, but we can set that within CircleCI's UI instead of customizing the CircleCI configs for these packs.

And this might be verging on bikeshedding, but I do think that we need a better name than SKIP_INDEX though. Something long like DO_NOT_ADD_TO_EXCHANGE_INDEX is a lot more descriptive over what the flag actually does. As it stands, a newbie may not understand that Exchange is just a slick UI against the index repository, and will have to dig a bit to figure out what that controls.

I do 100% agree that we should support deploying from a non-master branch name though. Not only because people are starting to switch to main instead of master, but also because it makes it more consistent if people install directly from a Git repo (st2 pack install https://github.com/StackStorm-Exchange/test2) versus installing from Exchange (st2 pack install test2). Right now, the first one would (I believe) install from the aaa branch, but the theoretical install from Exchange would install from the master branch. I'm not 100% sure about this, so please correct me if I'm wrong.

Also, I believe that the deploy step also collects some data about the pack when it generates the index (eg: number and name of actions, aliases, rules, policies, sensors, and triggers, as well as pack metadata from pack.yaml, the current pack version, and all available pack versions). So if the deploy step only collects data from master, then the content in the non-master default branch may not match what is advertised in Exchange.

[cognifloyd]

Cool.. I like the idea of using a variable added via CircleCI ui. I'll also rename the var as suggested.

[arm4b]

We're talking about index generation/deployment script here and not the pack installation.

The only 2 repositories that have non-default master branch (aaa for testing purposes) are test and test2. They're just for internal testing and we don't deploy these repositories to Exchange and don't plan to.

With that, non-master branch deployment would be just dead code if we merge it here. I suggest to split this into a separated PR if we may need it sometime in the future and focus on what's now.

[arm4b]

From what I understand @cognifloyd initially opened this PR when realized that test and test2 repos are failing to deploy to StackStorm Exchange index, until we discovered that these repositories should never present in that index.

So I'm not sure what we're trying to solve here.

[blag]

These two packs were failing to deploy due to an indirect and undocumented bug/"feature" that we may not want to have anymore - the restriction that the deploy step only publishes the master branch.

Now, I'm not presently aware of any Exchange pack that publishes from a non-master branch, so it may not feel like removing that restriction is very useful to anybody. But, as @cognifloyd pointed out, there is a movement in the open source community to use words that are more specifically descriptive and less mired in historical injustices than words that have been used in the past. In the Django project, the terms master/slave when referring to databases were renamed to leader/follower, which is not only more accurate but also have less baggage than the words "master" and "slave". Similarly, in the spirit of inclusivity, there is a movement to rename the "main" branch where development happens away from master to main.

Since we are now aware that the default branch names for many projects including our own might be changing in the future, it seems like a good time to remove the artificial restriction on what branches can be deployed to Exchange. As part of this, instead of relying on indirect and undocumented behavior to seemingly accidentally not deploy to Exchange, it's a good time to improve the code to explicitly skip deploying to Exchange, so future developers can instantly understand why these packs are not deploying to Exchange.

Just because we do not currently have any packs deploying from a non-master branch does not mean that it was always intended for that to be the case. For all we/I know, that could have been an oversight of previous maintainers. Now that it's explicit behavior it's easier to understand, reason about, and correct any future bugs in our deployment code.

[cognifloyd]

I opened this PR for several reasons. test packs are only part (albeit a large part) of those reasons.

1. I made quite a few changes to exchange CI recently, including (a) drop python2 tests, (b) prevent PAT tokens from expiring, (c) standardize CircleCI config across all packs as much as is feasible, etc.
2. The test and test2 packs would be ideal for testing changes like these to the CI infrastructure. That is the purpose listed in the README (see [1] below) for each of those packs. I could not use those packs, however, because ALL of the CircleCI config was out-of-date. They didn't even have CircleCI 2.0 config, and some of the changes were mistakenly added to the master branch. So, I had to use several other packs that were live on the exchange to test changes to the CI infrastructure which carried the risk of potentially breaking those packs if someone happened to install it during one of my tests.
3. The deploy workflow does more than just update the index. If we arbitrarily block the deploy workflow just to avoid updating the index, then I still have to use live packs to test changes to that deploy workflow. So, the test and test2 packs must run the deploy workflow as well to meet their purpose (see [1] below) of testing the CI infrastructure.
4. For the test packs, exclusion from the index was a quirky accident based on obscure misuse of the CI config. This PR makes it possible to explicitly exclude those packs from the index. And it does so in a way that should be clear and easy-to-understand for future contributors.
5. Resolve #109 by issuing a good error message as early as possible if a PAT has expired.
6. The test packs also explicitly state (see [1] below) that their purpose is to ensure non-master branches work with our CI infrastructure. So, this PR is a bugfix PR, not an enhancement.
7. The master branch limitation is arbitrary and unnecessary. As @blag described, many projects are moving from master to main for very good reasons (we don't want to chase off existing and potential contributors). This bugfix PR removes the arbitrary limitation around branch naming so that we can use other branches at any point.
8. (9, 10, 11, ...) Please re-read what @blag wrote. Using descriptive names instead of master is extremely important across the open source world and beyond. I used reasons 1-4 to cover other reasons--mostly technical--for this change, but caring about people is even more important! So my reason number 7 is far more important than any of the other reasons. Without people we have no project--no matter how technically awesome the project is.

[1] Here is the relevant section from the README of both test packs (test and test2):

NOTE: This pack is used for testing our pack Continous Integration system and uses a non-master default branch on Github (aaa) to verify that CI works with non-master default branches - please don't change.

[blag]

I have added the DO_NOT_ADD_TO_EXCHANGE_INDEX environment variable to the CircleCI project settings for stackstorm-test, and stackstorm-test2 and stackstorm-python3_test (see edit). It should have no effect on anything until this is merged.

Edit: @cognifloyd reminded me that the python3_test pack is actually installed directly from Exchange in end-to-end tests, so we do actually need to deploy that one. I have removed the DO_NOT_ADD_TO_EXCHANGE_INDEX environment variable in that CircleCI project.

[cognifloyd]

@armab is the reasoning behind this PR clearer now? If so, please approve and merge.