Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ITL: add all businessprocess module check parameters #9103

Merged
merged 11 commits into from
Apr 4, 2022
Merged

ITL: add all businessprocess module check parameters #9103

merged 11 commits into from
Apr 4, 2022

Conversation

log1-c
Copy link
Contributor

@log1-c log1-c commented Nov 29, 2021 •
edited by yhabteab
Loading

closes #9110
closes #9107

resolves #9103

@cla-bot
Copy link

cla-bot bot commented Nov 29, 2021

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @log1-c

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@log1-c log1-c changed the title add all businessprocess module check parameters ITL: add all businessprocess module check parameters Nov 29, 2021
@cla-bot
Copy link

cla-bot bot commented Nov 29, 2021

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @log1-c

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.

  • If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

@bobapple
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Nov 29, 2021
@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Dec 1, 2021
@Al2Klimov
Copy link
Member

Note: must be merged with squash-and-merge.

@tectumopticum
Copy link

naming of the defined variables should be consistent with the ones already in use, e.g.
"--state-type" = "$icingacli_businessprocess_statetype$"
I suggest e.g.
"--ack-is-ok" should be "$icingacli_businessprocess_ackisok" instead of "$icingacli_businessprocess_ack_is_ok" and so on (s. PR #9110)

Al2Klimov
Al2Klimov requested changes Dec 3, 2021
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Good point!

While you're on it, undo the alphabetical sorting of already not alphabetically sorted params.

@log1-c
Copy link
Contributor Author

log1-c commented Dec 3, 2021

changes done :)

@tectumopticum tectumopticum mentioned this pull request Dec 6, 2021
Closed
@Al2Klimov Al2Klimov requested a review from yhabteab February 21, 2022 12:11
yhabteab
yhabteab requested changes Feb 21, 2022
View reviewed changes
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

Can you also reorder argument state-type after the root-cause arg, shall be sorted alphabetically :) and update the documentation accordingly.

@log1-c
Copy link
Contributor Author

log1-c commented Feb 22, 2022

one doesn't want alphabetical sorting, the other does, not sure what is correct now, but here you go.
Docs updated as well.

@log1-c log1-c requested a review from yhabteab February 22, 2022 07:55
yhabteab
yhabteab previously approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

Sorry, didn't see it, that @Al2Klimov change requested you not to do that! But looks fine to me now.

@yhabteab yhabteab requested a review from Al2Klimov February 22, 2022 08:17
Al2Klimov
Al2Klimov requested changes Feb 23, 2022
View reviewed changes
doc/10-icinga-template-library.md Outdated Show resolved Hide resolved
doc/10-icinga-template-library.md Outdated Show resolved Hide resolved
itl/plugins-contrib.d/icingacli.conf Show resolved Hide resolved
itl/plugins-contrib.d/icingacli.conf Outdated Show resolved Hide resolved
itl/plugins-contrib.d/icingacli.conf Outdated Show resolved Hide resolved
itl/plugins-contrib.d/icingacli.conf Outdated Show resolved Hide resolved
@log1-c log1-c requested a review from Al2Klimov February 23, 2022 14:46
@Al2Klimov Al2Klimov removed their request for review February 24, 2022 11:15
@Al2Klimov Al2Klimov requested a review from yhabteab February 24, 2022 12:29
@Al2Klimov Al2Klimov merged commit 44a34cf into Icinga:master Apr 4, 2022
yhabteab pushed a commit that referenced this pull request Sep 5, 2022
@log1-c @yhabteab
Merge pull request #9103 from log1-c/patch-2
3393481 
ITL: add all businessprocess module check parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhabteab yhabteab yhabteab approved these changes

@Al2Klimov Al2Klimov Al2Klimov approved these changes

Assignees
No one assigned
Labels
cla/signed
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

CheckCommand "icingacli-businessprocess" adding new arguments to icingacli.conf
5 participants
@log1-c @bobapple @Al2Klimov @tectumopticum @yhabteab