genymobile/bitrise-step-genymotion-cloud-saas-start

bitrise-step-genymotion-cloud-saas-start's Issues

rename_on_conflict needs to be exposed

rename_on_conflict defaults to false which basically means you can only run a single instance of this step concurrently.

Bitrise review

Hi @thomascarpentier,

based on the review from the Bitrise Tooling team, I would like to leave some suggestions for this awesome step:

ISSUEs will block the Step to be shared into the Official Bitrise StepLib.
NOTEs: are just suggestions

step.yml

• NOTE: usually we do not leave template codes in the step.yml
• NOTE: for project_type_tags all cross-platform frameworks could be listed, as these types of Bitrise projects can have an Android native project. Based on this field we present or hide the step for a given Bitrise project. So I would suggest listing xamarin, react-native, cordova, ionic and flutter too.
• ISSUE: type_tags should hold only one type, in this case utility as it does not perform any test by itself
• ISSUE: please remove unused deps as deps are ensured by the Bitrise CLI, which takes unnecessary time, while the CLI activates the step if those dependencies are not used.
• ISSUE: gmcloud_saas_password should have the is_sensitive field set to true, to automatically redact the password from the build log.
• NOTE: I would suggest removing the gmcloud_saas_ prefix from the step inputs, as this way it needs more typing when configuring the step in a bitrise.yml file manually. Outputs are fine with the prefix as this way you can be sure that other outputs are not overriding these values.
• NOTE: descriptions are full sentences, so I would suggest using . at the end of the sentences.
• NOTE: gmcloud_saas_adb_serial_port's description could be a bit more structured, to make it easier to read. Two spaces mean a line break in markdown, something like:

If `gmcloud_saas_adb_serial_port` option is set,  
the instance will be connected to ADB on localhost:`gmcloud_saas_adb_serial_port`.  
Otherwise it will be connected to a port given by your system,  
`gmsaas instances list` to know which one.

bitrise.yml

• NOTE:

We usually add # Define these in your .bitrise.secrets.yml comment before the app level env vars, which needs to be defined in a Bitrise secrets file, to be able to test the step.

Other than that for the step inputs, like gmcloud_saas_email you set default value: GMCLOUD_SAAS_EMAIL, while the app level envvar's key is: GMCLOUD_SAAS_LOGIN.
We usually set the input's default env var as app-level env var and set the same value for it. This way if a secret is set with the given key, the app level env var will get the secret's value, which will be the given input's value. Like:

bitrise.yml

app:
  envs:
...
  # Define these in your .bitrise.secrets.yml
  - GMCLOUD_SAAS_EMAIL: $GMCLOUD_SAAS_EMAIL


workflows:
  test:
...
    - path::./:
        run_if: "true"
        inputs:
        - gmcloud_saas_email: $GMCLOUD_SAAS_EMAIL

.bitrise.secrets.yml

envs:
- GMCLOUD_SAAS_EMAIL: <my_email>

• NOTE: it worth to set the audit-this-step workflow as before_run for the primary test workflow, as it validates the step's step.yml.
• NOTE: I would suggest wiring in our go test steps.
  Those steps are enforcing go best practices, which will make the contribution easier for Go developers.

main.go

• ISSUE: while we do not preinstall the gmsaas tool on our virtual machines, the step should manage to ensure it is available on the host machine. Something like:

func ensureGMSAAS() error {
    _, installed := os.LookupEnv("gmsaas")
    if !installed {
        cmd := command.New("pip3", "install", "gmsaas")
        if out, err := cmd.RunAndReturnTrimmedCombinedOutput(); err != nil {
            return fmt.Errorf("%s failed, error: %s | output: %s", cmd.PrintableCommandArgs(), err, out)
        }
    }
    return nil
}

Also pip3 needs to be listed as deps in the step.yml

deps:
  brew:
  - name: python3
  - bin_name: pip3
  apt_get:
  - name: python3-pip
  - bin_name: pip3

• NOTE: you can use stepconf.Secret type for sensitive inputs. This way by default this var will be printed as ***** (even if Bitrise CLI secret filtering is disabled). To use the raw value of the secret you can use string(secretVar).
• NOTE: it worth to delegate errors to the clients in go functions and usually we only call failf (os.Exit(1)) only in the main function, so this way it is quite easy to follow where can the step exist and why.
• NOTE: step output export is implemented in the tools package.