ckeditor / ckeditor4-workflows-common

Shared CKEditor 4 GitHub workflows.
1 stars 2 forks source link

Config options in tests are not read properly #27

Open Dumluregn opened 3 years ago

Dumluregn commented 3 years ago

See https://github.com/ckeditor/workflow-tests-PR-6/runs/2676836704?check_suite_focus=true

In Read config step we have as a result Workflow config: {"setupWorkflows":{"pushAsPullRequest":"true"}}. The "setupWorkflows" should be stripped off and we should get a flat object {"pushAsPullRequest":"true"}. While this property is nested it is later inaccessible for the workflow (see that later in the workflow we have AS_PR: 0, which should be AS_PR: 1).

The problem only concerns tests - in ckeditor4 repo the config is read correctly: https://github.com/ckeditor/ckeditor4/runs/2681397796?check_suite_focus=true. The main suspicion is that the problem is caused because in ckeditor4 we have config in a json file:

{
  "setupWorkflows": {
    "pushAsPullRequest": true
  }
}

and in tests config is in JS file:

config: {
    'setupWorkflows': {
        'pushAsPullRequest': 'true'
    }
}

and probably it isn't correctly converted.

Dumluregn commented 3 years ago

I see the problem is a bit simpler. In Read config step we only strip the workflow name if config comes from the workflows-config.json file and not if it is passed as an input:

CONFIG='{}'
if [[ ! -z '${{ github.event.inputs.config }}' ]]; then
  CONFIG='${{ github.event.inputs.config }}'
elif [[ -f "./.github/workflows-config.json" ]]; then
  CONFIG=$( jq -c .setupWorkflows './.github/workflows-config.json' )
fi

So in test fixtures we should either:

1) only pass the config options without a config name 2) or also perform config stripping if config is passed directly.

The latter seems a bit pointless because we tell exactly which workflow gets given options so I think we can go with the first solution.

f1ames commented 3 years ago

There is already a PR in progress covering that issue, see https://github.com/ckeditor/ckeditor4-workflows-common/pull/28.

sculpt0r commented 3 years ago

❄️ Let's wait with this issue - until we decide if the current bot-updates flow is really unpleasant.