Introduction of broker config cli and much more! #317

JacobCallahan · 2024-09-17T18:01:17Z

The initial change of this was to introduce the broker config cli. However, to do this, a number of supporting and related changes needed to be made.

The first of these is the new ConfigManager class that manages reading from and writing to the config file.
Since so much logic was implemented for this, it made more sense to roll in the import/init functionality, that was originally in the settings module, into the new class.

This also gave me the opportunity to separate the test config from the example config. This removal, and other changes in this were supported by ConfigManager's migration functionality.

To simplify the nested chunk notation, I moved provider instances out from a list to just being a nested dictionary. This actually simplified the instance logic quite a bit.

Related to config simlification was the separation of the host settings to a new ssh chunk of the config. This is because not all users use the host functionality Broker provides. A likely change with this release is going to be removing a default ssh backend from Broker's requirements.

While I was at it, I also switched the CLI over to rich-click as part of a larger effort to improve the CLI experience.

JacobCallahan · 2024-09-17T20:03:52Z

broker/config_manager.py

+
+    def restore(self):
+        """Restore the configuration file from a backup if it exists."""
+        logger.debug(


I'm debating whether this should be info level or not. There is a stronger case for this being info level than the message in backup.

JacobCallahan · 2024-09-17T20:07:10Z

broker/config_manager.py

+            return curr_chunk
+        if C_SEP in chunk:
+            curr, chunk = chunk.split(C_SEP, 1)
+            # curr = int(curr) if curr.isdigit() else curr


This is a remnant from when I allowed traversing lists, like how instances are currently stored. Could keep it in for some future use or just get rid of these lines.

broker/config_manager.py

JacobCallahan · 2024-09-17T20:53:19Z

broker/hosts.py

+        global SETTINGS_VALIDATED  # noqa: PLW0603
+        if not SETTINGS_VALIDATED:
+            logger.debug("Validating ssh settings")
+            settings.validators.validate(only="SSH")
+            SETTINGS_VALIDATED = True


delay validation until needed to avoid requiring the ssh config chunk.
this isn't currently important since every setting under ssh has a default value defined in its validator.

broker/hosts.py

JacobCallahan · 2024-09-17T20:57:05Z

broker/settings.py

+        if click.confirm("Would you like to run the migrations now?"):
+            cfg_manager.migrate()


right now this is a choice, but we could also just remove the choice and make it automatic.

The initial change of this was to introduce the broker config cli. However, to do this, a number of supporting and related changes needed to be made. The first of these is the new ConfigManager class that manages reading from and writing to the config file. Since so much logic was implemented for this, it made more sense to roll in the import/init functionality, that was originally in the settings module, into the new class. This also gave me the opportunity to separate the test config from the example config. This removal, and other changes in this were supported by ConfigManager's migration functionality. To simplify the nested chunk notation, I moved provider instances out from a list to just being a nested dictionary. This actually simplified the instance logic quite a bit. Related to config simlification was the separation of the host settings to a new ssh chunk of the config. This is because not all users use the host functionality Broker provides. A likely change with this release is going to be removing a default ssh backend from Broker's requirements. While I was at it, I also switched the CLI over to rich-click as part of a larger effort to improve the CLI experience.

Due to timing, I couldn't cleanly include this in the previous commit, but here it is. Now Broker actions, including checkins will reference this new setting when determining how many threads it should use.

JacobCallahan · 2024-09-24T22:52:19Z

@tpapaioa @ogajduse I'd like to have this merged this week; any thoughts?

With this change, users can now specify a `--from` argument with a valid path or url to a valid broker settings file. This will still try to fallback on the current behavior of using a local repo or the main SatelliteQE/broker repo as the source.

Broker has expanded a while ago with the types of hosts it can support, it's time we updated our terminology to match.

ogajduse

This is a partial review, I went through a quarter of the patch and left some comments.

broker/commands.py

broker/config_manager.py

A number of changes were needed both functionally and stylistically to get this where we want it, but it's pretty good now. I don't know if I like the "Validation passed!" message, since it will likely print every time, but it can stay for now.

broker/providers/ansible_tower.py

broker/config_manager.py

broker/providers/ansible_tower.py

ogajduse · 2024-09-30T12:07:01Z

broker/settings.py

 BROKER_DIRECTORY = Path.home().joinpath(".broker")
+TEST_MODE = "pytest" in sys.modules


Did you test that pytest is not in sys.modules when broker is imported in robottelo or a similar project?

it almost certainly should trigger test mode, so we wouldn't automatically migrate while in a pytest session.

Let me rephrase, do we want to have TEST_MODE set to True when we run robottelo (Foreman) tests? If we don't, this logic needs to be changed.
pytest --setup-plan tests/foreman/api/test_ping.py -s

[74] > /home/ogajduse/repos/broker/broker/settings.py(28)<module>() -> TEST_MODE = "pytest" in sys.modules 2 frames hidden (try 'help hidden_frames') (Pdb++) l 23 from broker.exceptions import ConfigurationError 24 25 INTERACTIVE_MODE = ConfigManager.interactive_mode 26 BROKER_DIRECTORY = Path.home().joinpath(".broker") 27 breakpoint() 28 -> TEST_MODE = "pytest" in sys.modules 29 30 if TEST_MODE: # when in test mode, don't use the real broker directory 31 BROKER_DIRECTORY = Path("tests/data/") 32 elif "BROKER_DIRECTORY" in os.environ: 33 envar_location = Path(os.environ["BROKER_DIRECTORY"]) (Pdb++) "pytest" in sys.modules True

tests/test_config_manager.py

broker/config_manager.py

broker/session.py

PyYaml is a good library, but had the unfortunate side-effect of stripping comments from yaml files on read/write. Switching to ruamel now let's us retain the comments, and arguably improves usage in some ways. There may be some issues with awxkit objects due to the way the representers are added, but I can't say for certain that is the case yet.

JacobCallahan added enhancement New feature or request 0.6 Things aligned to the 0.6 release labels Sep 17, 2024

JacobCallahan force-pushed the config_manager branch from 7290f31 to 032372a Compare September 17, 2024 18:05

JacobCallahan requested review from tpapaioa and ogajduse September 17, 2024 19:22

JacobCallahan commented Sep 17, 2024

View reviewed changes

broker/config_manager.py Outdated Show resolved Hide resolved

JacobCallahan commented Sep 17, 2024

View reviewed changes

broker/hosts.py Show resolved Hide resolved

JacobCallahan commented Sep 17, 2024

View reviewed changes

JacobCallahan force-pushed the config_manager branch 6 times, most recently from b309886 to 9b17fed Compare September 18, 2024 04:59

JacobCallahan marked this pull request as ready for review September 18, 2024 18:40

JacobCallahan added 2 commits September 20, 2024 15:12

Respect the thread_limit setting included in the previous commit

37eb078

Due to timing, I couldn't cleanly include this in the previous commit, but here it is. Now Broker actions, including checkins will reference this new setting when determining how many threads it should use.

JacobCallahan force-pushed the config_manager branch from 9b17fed to 37eb078 Compare September 20, 2024 20:21

JacobCallahan deleted the branch SatelliteQE:0.6 September 20, 2024 20:23

JacobCallahan closed this Sep 20, 2024

JacobCallahan reopened this Sep 20, 2024

JacobCallahan force-pushed the config_manager branch from 922833e to f130b74 Compare September 26, 2024 18:35

Change cli terminology from VMs to Hosts

45efe37

Broker has expanded a while ago with the types of hosts it can support, it's time we updated our terminology to match.

JacobCallahan force-pushed the config_manager branch from f787d37 to 45efe37 Compare September 27, 2024 14:53

ogajduse reviewed Sep 27, 2024

View reviewed changes

broker/commands.py Outdated Show resolved Hide resolved

broker/config_manager.py Outdated Show resolved Hide resolved

broker/config_manager.py Show resolved Hide resolved

tpapaioa approved these changes Sep 27, 2024

View reviewed changes

Rework config manager's validation

0f937ae

A number of changes were needed both functionally and stylistically to get this where we want it, but it's pretty good now. I don't know if I like the "Validation passed!" message, since it will likely print every time, but it can stay for now.

JacobCallahan force-pushed the config_manager branch 2 times, most recently from 7bdbd02 to bac3074 Compare September 27, 2024 21:34

JacobCallahan commented Sep 27, 2024

View reviewed changes

broker/providers/ansible_tower.py Outdated Show resolved Hide resolved

ogajduse reviewed Sep 30, 2024

View reviewed changes

JacobCallahan force-pushed the config_manager branch 2 times, most recently from 047b887 to 107f7b3 Compare September 30, 2024 16:04

tpapaioa requested changes Oct 1, 2024

View reviewed changes

broker/session.py Outdated Show resolved Hide resolved

JacobCallahan force-pushed the config_manager branch from 107f7b3 to c7b95e3 Compare October 1, 2024 17:42

JacobCallahan requested a review from tpapaioa October 1, 2024 17:51

ogajduse approved these changes Oct 1, 2024

View reviewed changes

JacobCallahan force-pushed the config_manager branch from c7b95e3 to 23a7025 Compare October 2, 2024 15:54

JacobCallahan merged commit 4c9a6dc into SatelliteQE:0.6 Oct 3, 2024
4 checks passed

JacobCallahan deleted the config_manager branch October 3, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduction of broker config cli and much more! #317

Introduction of broker config cli and much more! #317

JacobCallahan commented Sep 17, 2024

JacobCallahan Sep 17, 2024

JacobCallahan Sep 17, 2024

JacobCallahan Sep 17, 2024

JacobCallahan Sep 17, 2024

JacobCallahan commented Sep 24, 2024

ogajduse left a comment

ogajduse Sep 30, 2024

JacobCallahan Sep 30, 2024

ogajduse Oct 1, 2024

		if click.confirm("Would you like to run the migrations now?"):
		cfg_manager.migrate()

		BROKER_DIRECTORY = Path.home().joinpath(".broker")
		TEST_MODE = "pytest" in sys.modules

Introduction of broker config cli and much more! #317

Introduction of broker config cli and much more! #317

Conversation

JacobCallahan commented Sep 17, 2024

JacobCallahan Sep 17, 2024

Choose a reason for hiding this comment

JacobCallahan Sep 17, 2024

Choose a reason for hiding this comment

JacobCallahan Sep 17, 2024

Choose a reason for hiding this comment

JacobCallahan Sep 17, 2024

Choose a reason for hiding this comment

JacobCallahan commented Sep 24, 2024

ogajduse left a comment

Choose a reason for hiding this comment

ogajduse Sep 30, 2024

Choose a reason for hiding this comment

JacobCallahan Sep 30, 2024

Choose a reason for hiding this comment

ogajduse Oct 1, 2024

Choose a reason for hiding this comment