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.

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

CLI: make refresh_cache and request_timeout params global #884

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

neomilium
Copy link
Contributor

Considering the name of these parameters, they should be affect system-wide configuration but were set in root configuration file.
This commit fixes this unexpected behavior.

@neomilium neomilium changed the title Fix refresh_cache and request_timeout parameters usage CLI: Fix refresh_cache and request_timeout parameters usage Sep 25, 2020
@neomilium
Copy link
Contributor Author

Spec tests failed due to two reasons:

  • I need to test that these parameters affect the right file (WIP)
  • Some tests strictly verify that there are only the tested parameters, no more

IMHO, two possible behaviors, the configuration file can be filled by

  • specified parameters only (so, I will allow undef on refresh_cache and refresh_timeout)
  • all parameters, ie. including refresh_cache and refresh_timeout (so, I will add them to attempted configuration entries)

The question is what the foreman team prefers?

Note: If someone have a better direction, show me the way ;-)

@neomilium neomilium changed the title CLI: Fix refresh_cache and request_timeout parameters usage WIP: CLI: Fix refresh_cache and request_timeout parameters usage Sep 26, 2020
@neomilium neomilium marked this pull request as draft October 15, 2020 10:23
@neomilium
Copy link
Contributor Author

@ekohl Do you have any point of view on this? Who should I reach in Foreman about this topic?

@ekohl
Copy link
Member

ekohl commented Oct 15, 2020

I would accept this change (provided tests pass).

Considering the name of these parameters, they should be affect system-wide configuration but were set in root configuration file.
This commit fixes this unexpected behavior.
@neomilium neomilium marked this pull request as ready for review November 22, 2020 13:43
@neomilium neomilium changed the title WIP: CLI: Fix refresh_cache and request_timeout parameters usage CLI: Fix refresh_cache and request_timeout parameters usage Nov 22, 2020
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks!

@ekohl ekohl changed the title CLI: Fix refresh_cache and request_timeout parameters usage CLI: make refresh_cache and request_timeout params global Nov 23, 2020
@ekohl ekohl merged commit f0b9b08 into theforeman:master Nov 23, 2020
@neomilium neomilium deleted the fix-cli-refresh-params branch November 23, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants