Skip to content

Conversation

jbostoen
Copy link

@jbostoen jbostoen commented Sep 8, 2025

Base information

Question Answer
Related to a SourceForge thread / Another PR / Combodo ticket? No
Type of change? Enhancement

Symptom (bug) / Objective (enhancement)

  • The iTop API natively supports the use of the switch_env parameter.
  • The data base collector should also support this.

Proposed solution (bug and enhancement)

  • An optional parameter that defaults to production .

Checklist before requesting a review

  • I have performed a self-review of my code, and that it's compliant with Combodo's guidelines
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • I have made sure the PR is clear and detailled enough so anyone can understand the real purpose without digging in the code

Checklist of things to do before PR is ready to merge

No new phpunit test as I don't know what environments Combodo has in place.

<itop_password>admin</itop_password>
<itop_token/>
<itop_login_mode/>
<itop_env>production</itop_env>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this corner case should be present in each collectors default sample config.

Maybe make it similar as for login mode?

Suggested change
<itop_env>production</itop_env>
<itop_env/>

Copy link
Author

Choose a reason for hiding this comment

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

Production is the default anyway - both when using iTop, or also as the "non configured" value.
I don't think it makes a big difference whether we clearly set the (default) environment name in the XML, or do it your way, or perhaps remove it altogether from the XML (then it's just less clear it's supported).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally would have removed it, but the suggestion I made is not "My way", it is the way Combodo decided to add parameters not widely used (like itop_login_mode in #29).

So it is just a suggestion, Combodo review team can decide what to do.

@jf-cbd
Copy link
Member

jf-cbd commented Sep 26, 2025

Hey @jbostoen, thanks for your PR. It seems functionally interesting, but we'll have to see technically if it doesn't bring problems with tests environment created by the designer.

@jf-cbd jf-cbd moved this from First review needed to Pending technical review in Combodo PRs dashboard Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending technical review
Development

Successfully merging this pull request may close these issues.

3 participants