deployImages assumes that options.branch is always main

[samreid]

Discovered in https://github.com/phetsims/perennial/issues/240, @zepumph and I saw this code in deployImages:

const deployImages = async options => {
  console.log( `deploying images with branch ${options.branch}, brands ${options.brands}` );
  if ( options.branch ) {
    await gitCheckout( 'chipper', options.branch );
    await gitPull( 'chipper' );
    await execute( 'npm', [ 'prune' ], chipperDir );
    await execute( 'npm', [ 'update' ], chipperDir );

    await gitCheckout( 'perennial-alias', options.branch );
    await gitPull( 'perennial-alias' );
    await execute( 'npm', [ 'prune' ], perennialAliasDir );
    await execute( 'npm', [ 'update' ], perennialAliasDir );
  }

  if ( options.simulation && options.version ) {
    await processSim( options.simulation, options.brands, options.version );
  }

Note that we check out options.branch for chipper and perennial-alias. However, immediately afterwards in processSim we run :

await execute( 'grunt', [ 'checkout-master-all' ], './' );

Therefore, it seems we may want to assert that options.branch is master when running deployImages. @mattpen does that sound right to you? Or if we do need options.branch, then how does that relate to checking out master of all repos immediately afterwards?

Here is a patch we considered that asserts that if the branch is provided, it must be master--how does this look to you? Or can options.branch be deleted altogether?

```diff Index: js/build-server/deployImages.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/build-server/deployImages.js b/js/build-server/deployImages.js --- a/js/build-server/deployImages.js (revision 431586ed334a85a6b2efa16e0f778a0443405d13) +++ b/js/build-server/deployImages.js (date 1634588321887) @@ -6,6 +6,7 @@ const constants = require( './constants' ); const fs = require( 'fs' ); const axios = require( 'axios' ); +const assert = require( 'assert' ); const chipperDir = '../chipper'; const perennialAliasDir = '../perennial-alias'; @@ -66,6 +67,7 @@ const deployImages = async options => { console.log( `deploying images with branch ${options.branch}, brands ${options.brands}` ); if ( options.branch ) { + assert( options.branch === 'master', 'deployImages should be run on master branch' ); await gitCheckout( 'chipper', options.branch ); await gitPull( 'chipper' ); await execute( 'npm', [ 'prune' ], chipperDir ); ```

We also saw this command line interface:

  grunt.registerTask( 'deploy-images',
    'Rebuilds all images for all sims\n' +
    '--branch : The chipper branch to use for image generation\n' +
    '--brands : A comma-separated list of brand names to deploy, currently only phet supported',
    wrapTask( async () => {

But note that it tries to check out the same branch name on chipper and perennial-alias at the moment. We aren't sure what this function is used for, do you think it should be changed?

[mattpen]

Here is a patch we considered that asserts that if the branch is provided, it must be master--how does this look to you?

I think this is fine for now, and the patch looks good to me. In the future, we may want to support more flexibility in terms of which branch we use, but for now only master is supported.

We aren't sure what this function is used for, do you think it should be changed?

The grunt function triggers a build-server procedure that redeploys all images for all sims from master to the phet brand production releases, for when we make an image configuration change that should affect all sims in production but not trigger a maintenance release. We should probably remove the branch and brands options.

[mattpen]

@samreid @zepumph - anything else to do here?

EDIT: removed accidental tag of CM

[samreid]

Should it be an error to call deployImages without options.branch? When would we want to run the last half of deployImages without preparing the working copy? Also, the call site doesn't mention that "branch" is an option:

/**
 * taskQueue ensures that only one build/deploy process will be happening at the same time.  The main build/deploy logic is here.
 *
 * @param {Object}
 * @property {JSON} repos
 * @property {String} api
 * @property {String} locales - comma separated list of locale codes
 * @property {String} simName - lower case simulation name used for creating files/directories
 * @property {String} version - sim version identifier string
 * @property {String} servers - deployment targets, subset of [ 'dev', 'production' ]
 * @property {string[]} brands - deployment brands
 * @property {String} email - used for sending notifications about success/failure
 * @property {String} translatorId - rosetta user id for adding translators to the website
 * @property {String} res - express response object
 * @property {winston} winston - logger
 */
async function taskWorker( options ) {
  if ( options.deployImages ) {
    try {
      await deployImages( options );

[zepumph]

What about instead we just remove and notion of options.branch from this task, and hard code master? That is probably the clearest, and it will have the same behavior. With this let's add documentation that deployImages with checkout master, since it already doing that work anyways. Let's just document it and clean up a bit.

[samreid]

That sounds reasonable to me, but I would recommend @mattpen take the lead or confirm, thanks!