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

make custom fields configurable #79

Merged
merged 3 commits into from
Feb 23, 2023
Merged

make custom fields configurable #79

merged 3 commits into from
Feb 23, 2023

Conversation

dleusing
Copy link
Contributor

No description provided.

@cla-bot
Copy link

cla-bot bot commented Apr 11, 2022

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: @dleusing

  • 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.

@dleusing
Copy link
Contributor Author

CLA signed.

@bobapple
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Apr 11, 2022
dleusing
dleusing commented Apr 11, 2022
View reviewed changes
Copy link
Contributor Author

@dleusing dleusing left a comment

Choose a reason for hiding this comment

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

fix for Whitespace found at end of line

Thomas-Gelf
Thomas-Gelf requested changes Jun 27, 2022
View reviewed changes
Copy link
Collaborator

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

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

Hi @dleusing,

thank you for this contribution! I added a few small inline change requests, could you please address them?

Thanks
Thomas

application/clicommands/SendCommand.php Outdated Show resolved Hide resolved
doc/03-Configuration.md Show resolved Hide resolved
library/Jira/IssueTemplate.php Outdated
@@ -127,13 +127,16 @@ protected function getCustomFields()

protected function getDefaultFields()
{
$config = Config::module('jira');
$Key = $config->get('jira_key_fields', 'field_icingaKey', 'icingaKey');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please start variable names with a lowercase letter, $Key VS $key, $Status -> $status

library/Jira/RestApi.php Outdated Show resolved Hide resolved
library/Jira/IssueTemplate.php Outdated
@@ -127,13 +127,16 @@ protected function getCustomFields()

protected function getDefaultFields()
{
$config = Config::module('jira');
$Key = $config->get('jira_key_fields', 'field_icingaKey', 'icingaKey');
$Status = $config->get('jira_key_fields', 'field_icingaStatus', 'customfield_19220');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hard-coded custom field identifier, see above

dleusing
dleusing commented Jun 28, 2022
View reviewed changes
Copy link
Contributor Author

@dleusing dleusing left a comment

Choose a reason for hiding this comment

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

Review finished

@fl0wx
Copy link

fl0wx commented Nov 11, 2022

@Thomas-Gelf any updates on the review ?

@Thomas-Gelf
Copy link
Collaborator

@dleusing, @fl0wx: I'm sorry, I overlooked that this is now ready. Please re-check your config lookups, it seems that there are some inconsistencies, field_icingaStatus VS icingaStatus and jira_key_fields VS ui. The rest looks good to me. Once the lookups have been fixed, this should be ready for merge. It would also be great if you could squash (and force-push) your changes.

Thanks,
Thomas

dleusing
dleusing commented Nov 12, 2022
View reviewed changes
Copy link
Contributor Author

@dleusing dleusing left a comment

Choose a reason for hiding this comment

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

reviewed

@dleusing
Copy link
Contributor Author

changes are made

@fl0wx
Copy link

fl0wx commented Nov 28, 2022

@Thomas-Gelf still anything open or can this be merged?

@fl0wx
Copy link

fl0wx commented Dec 15, 2022

@Thomas-Gelf any updates?

@lippserd
Copy link
Member

@Thomas-Gelf any updates?

We'll have a look.

@lippserd lippserd requested review from raviks789 and removed request for Thomas-Gelf December 15, 2022 16:07
Thomas-Gelf
Thomas-Gelf approved these changes Dec 16, 2022
View reviewed changes
Copy link
Collaborator

@Thomas-Gelf Thomas-Gelf left a comment

Choose a reason for hiding this comment

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

Now it looks good to me, thank you!

raviks789
raviks789 reviewed Dec 19, 2022
View reviewed changes
Copy link
Contributor

@raviks789 raviks789 left a comment

Choose a reason for hiding this comment

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

@dleusing, it would be great if you could clean up (squash the commits and force-push) the commits as @gelf requested before. It would be nice to have one commit per topic (file, function etc).

@puxilein
Copy link

@dleusing maybe as christmas present? :-)

@puxilein
Copy link

puxilein commented Jan 9, 2023

@dleusing any news?

@fl0wx
Copy link

fl0wx commented Jan 9, 2023

@dleusing : when can we expect the requested rework?
@Thomas-Gelf @raviks789 : Would it be possible to merge it anyway if dleusing wont react?

@lippserd
Copy link
Member

lippserd commented Jan 9, 2023

@dleusing : when can we expect the requested rework? @Thomas-Gelf @raviks789 : Would it be possible to merge it anyway if dleusing wont react?

We can also make the changes.

@puxilein
Copy link

puxilein commented Jan 9, 2023

@lippserd that would be great! 👍

@raviks789 raviks789 force-pushed the master branch 2 times, most recently from 14edb0a to 2aa9f51 Compare January 9, 2023 15:59
@fl0wx
Copy link

fl0wx commented Jan 12, 2023

@Thomas-Gelf @raviks789 thanks a lot, can you also give an estimated time for the merge & release of a new version?

@puxilein
Copy link

Any news?

@verboEse verboEse mentioned this pull request Feb 8, 2023
Closed
@puxilein
Copy link

*ping :-)
@Thomas-Gelf @raviks789 can you give an estimated time for the merge and release of the new version?

@fl0wx
Copy link

fl0wx commented Feb 17, 2023

@lippserd @Thomas-Gelf @raviks789 any update?
" thanks a lot, can you also give an estimated time for the merge & release of a new version? "

@lippserd
Copy link
Member

Stop spamming.

@nilmerg nilmerg added the enhancement New feature or request label Feb 20, 2023
@nilmerg nilmerg added this to the v1.3.0 milestone Feb 20, 2023
@dleusing @raviks789
Add documentation for configurable custom fields
f1b1761
@dleusing @raviks789
Use the configured custom fields
0e1fbaf 
Since the custom fields are made configurable.
The configured custom fields `icingaKey` and `icingaStatus` are used where necessary.
@raviks789 raviks789 merged commit 6914b06 into Icinga:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomas-Gelf Thomas-Gelf Thomas-Gelf approved these changes

@raviks789 raviks789 raviks789 approved these changes

Assignees

@raviks789 raviks789

Labels
cla/signed enhancement New feature or request
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
8 participants
@dleusing @bobapple @fl0wx @Thomas-Gelf @lippserd @puxilein @raviks789 @nilmerg