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

Add an option run checks only and exit #372

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Apr 10, 2024

The intent is to at least allow two scenarios:

  1. Users that want to be able to run checks before committing to a full install
  2. Allowing foreman-maintain to run the installer checks as part of it's health checks rather than re-implementing the checks

@evgeni
Copy link
Member

evgeni commented Apr 11, 2024

Didn't we discuss (2) and end up with "this introduces a chicken-egg situation, where people want to run checks before they upgrade, but the new checks require the new installer to be present"?

I'm cool with the feature as such, just wonder if we're really getting (2) out of it.

@ehelms
Copy link
Member Author

ehelms commented Apr 11, 2024

Didn't we discuss (2) and end up with "this introduces a chicken-egg situation, where people want to run checks before they upgrade, but the new checks require the new installer to be present"?

I'm cool with the feature as such, just wonder if we're really getting (2) out of it.

Correct - this won't help with new checks in the installer. I was thinking about existing checks that are more persistent? Specifically, I have seen multiple requests to have foreman-maintain health check do a hostname check. I suppose I could just copy that code into foreman-maintain but thought I'd consider this route first and see what others think.

@evgeni
Copy link
Member

evgeni commented Apr 11, 2024

Oh, okay, that it would help with.

@ehelms
Copy link
Member Author

ehelms commented Apr 11, 2024

@ekohl Any thoughts to adding this feature?

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.

Overall I think this makes sense. Some small implementation details inline.

lib/kafo/kafo_configure.rb Outdated Show resolved Hide resolved
self.class.exit(:invalid_system)
else
logger.notice("System checks passed")
Copy link
Member

Choose a reason for hiding this comment

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

I'd argue this is info, not notice. https://en.wikipedia.org/wiki/Syslog#Severity_level says for notice:

Conditions that are not error conditions, but that may require special handling.

And info:

Confirmation that the program is working as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem we always have is that we had to use NOTICE because of how puppet uses INFO and that forced our hand to break some of these rules in order to provide users with useful information. Notice is our default, and I believe that this is a useful output for default state, especially when running checks only or the output feels like nothing actually happened:

2024-04-18 14:08:27 [NOTICE] [root] Loading installer configuration. This will take some time.
2024-04-18 14:08:32 [NOTICE] [root] Running installer with log based terminal output at level NOTICE.
2024-04-18 14:08:32 [NOTICE] [root] Use -l to set the terminal output log level to ERROR, WARN, NOTICE, INFO, or DEBUG. See --full-help for definitions.
2024-04-18 14:08:33 [NOTICE] [checks] System checks passed

vs.

2024-04-18 14:08:27 [NOTICE] [root] Loading installer configuration. This will take some time.
2024-04-18 14:08:32 [NOTICE] [root] Running installer with log based terminal output at level NOTICE.
2024-04-18 14:08:32 [NOTICE] [root] Use -l to set the terminal output log level to ERROR, WARN, NOTICE, INFO, or DEBUG. See --full-help for definitions.

@ehelms ehelms force-pushed the allow-just-checks branch from 34d7d23 to 713d777 Compare April 18, 2024 18:09
@ehelms ehelms requested a review from ekohl April 19, 2024 12:58
@ekohl ekohl merged commit 01f6285 into theforeman:master Apr 19, 2024
6 checks passed
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