Skip to content

Not all runners (e.g. orchestra) have runner_parameters#750

Merged
lakshmi-kannan merged 1 commit intomasterfrom
fix_ci_break
Jun 20, 2018
Merged

Not all runners (e.g. orchestra) have runner_parameters#750
lakshmi-kannan merged 1 commit intomasterfrom
fix_ci_break

Conversation

@lakshmi-kannan
Copy link
Contributor

@lakshmi-kannan lakshmi-kannan commented Jun 20, 2018

With the orchestra changes merged to st2 master, the script to generate runner documentation was failing. Reason: orchestra runner doesn't have 'runner_parameters' section. This PR makes the script work with runners with no runner_parameters.


with open(destination_path, 'w') as fp:
fp.write(result)
runner_parameters = runner.get('runner_parameters', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I would prefer runner.get('runner_parameters', {}), then we can get rid of if and one level of indentation :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't work in this case since the file generation and writing etc is needed only when runner_parameters are present but otherwise your suggestion is valid.

@arm4b
Copy link
Member

arm4b commented Jun 20, 2018

Would be better to communicate in PR description more nicely what it is and why it's needed.

I dig through links that this PR fixes the st2docs build, but others might guess on our cryptic context-hidden changes.

@lakshmi-kannan
Copy link
Contributor Author

@armab Sorry, I meant to type it in but laziness got the better of me.

@lakshmi-kannan lakshmi-kannan merged commit 44e4c8d into master Jun 20, 2018
@arm4b arm4b deleted the fix_ci_break branch June 20, 2018 15:26
@Kami Kami added this to the 2.8.0 milestone Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants