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

Foreman broker #291

Merged
merged 6 commits into from
May 21, 2024
Merged

Foreman broker #291

merged 6 commits into from
May 21, 2024

Conversation

knoppi
Copy link
Contributor

@knoppi knoppi commented May 3, 2024

As dicussed in #284 we created a draft for a foreman broker. Command execution relies on foreman's remote execution.

No configuration of foreman is done by this broker and host provisioning relies solely on correctly configured host groups. The broker can understand the parameters image and compute_resource to do an image based deployment which accelerates the checkout process.

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Overall great job with this addition, I'm very impressed!
I mostly just have some organizational requests.

broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Outdated Show resolved Hide resolved
help="Name of the Foreman hostgroup to deploy the host",
),
]
_execute_options = []
Copy link
Member

Choose a reason for hiding this comment

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

were you planning on using the job invocation for executes at this time or a later time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is implemented to use job invocation for executes and I already tried it within robottelo tests (mediated through the created host object).

I had no use for implementing this as a standalone feature for the command line so far. Or am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

I have no solid requirement for hooking up the execute functionality through the cli and Broker's execute method. I was primarily just thinking that it looks like you have everything needed here to set that up.

broker/providers/foreman.py Outdated Show resolved Hide resolved
broker/providers/foreman.py Show resolved Hide resolved
@dosas dosas mentioned this pull request May 15, 2024
@knoppi knoppi force-pushed the foreman-broker branch 2 times, most recently from 949cfce to 5c3f89c Compare May 15, 2024 09:51
@knoppi knoppi requested a review from JacobCallahan May 15, 2024 09:53
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Thanks for the quick updates to this PR and the great effort overall on it!

@JacobCallahan
Copy link
Member

@knoppi It looks like your test failure is due to a chicken/egg design limitation around Broker's config that I can likely address for 0.5. I assume these tests are passing locally.

@knoppi
Copy link
Contributor Author

knoppi commented May 16, 2024

@knoppi It looks like your test failure is due to a chicken/egg design limitation around Broker's config that I can likely address for 0.5. I assume these tests are passing locally.

There might be some design issue with my test as well. They were passing locally when I ran them with a non-working, but valid config. Yesterday when reworking I realized they fail without pointing to my test settings. They fail during validation because they cannot find the required settings (organization, location, username, password).

I then checked how you do this for other providers. For containers there is no setting defined as must_exist and you always have a default to fall-back to. For Ansible Tower the constructor never gets called.

My preferred solution would be reading the settings in the class ForemanBind where they are actually needed, so I could overwrite the validators in the MockStub. With the current design, I see two options:

  1. Get rid of tests that rely on the constructor.
  2. Store my test settings as defaults within the validators.

Option 1 would leave me rather useless tests. Option 2 seems error-prone but error handling should be sufficient to indicate if you are using the wrong values or forgot to set some.

Do you have an oppinion on that?

@JacobCallahan
Copy link
Member

Since you're loading the settings in the init phase, you also have the option to pass a MockStub version of the settings that has the values you want for your test.

    ...
    settings = kwargs.pop("stub_settings", settings)

However, since most of your validators are set to must_exist, you'd still fail dynaconf's settings validation. Hold tight for now and we'll see if the change I'm working on for local settings init will help.

@JacobCallahan
Copy link
Member

@knoppi I've proposed #293 that will hopefully solve this issue in GitHub actions. If you don't mind checking it over, I can merge it and then a rebase of this PR will test if it works as expected for this.

@JacobCallahan
Copy link
Member

@knoppi I've merged #293, so please rebase this PR and push back up. I'll then kick off the actions and see how the tests work.

Jan Bundesmann added 5 commits May 21, 2024 09:39
- Rename ForemanAPI to ForemanBind
- Put Foreman Bind in a separate file
- Create exception for FormanBind
- Make internal method private
- Add method to query hostgroup information
@knoppi
Copy link
Contributor Author

knoppi commented May 21, 2024

Hi, I'm not sure if the tests work after the rebase.

Locally they would never take the settings from the repo. init_settings_from_local_repo would search in /lib/python3.11/site-packages/ for the example file (relative to the included file which resides in /lib/python3.11/site-packages/broker/.... So they always fall-back to downloading from github and then will not contain settings for foreman.

In my latest commit you will find a workaround. With that, tests pass.

@knoppi knoppi requested a review from JacobCallahan May 21, 2024 10:59
Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

ACK. Thanks for following this all through and adding the new provider!

@JacobCallahan JacobCallahan merged commit b7e059b into SatelliteQE:0.5 May 21, 2024
4 checks passed
JacobCallahan pushed a commit that referenced this pull request May 29, 2024
* Add foreman broker

* add example settings for Foreman broker

* Reorganize classes

- Rename ForemanAPI to ForemanBind
- Put Foreman Bind in a separate file
- Create exception for FormanBind
- Make internal method private
- Add method to query hostgroup information

* Apply organizational changes in tests too

* Get rid of constructor call in provider_help

* Hotfix for github action

---------

Co-authored-by: Jan Bundesmann <[email protected]>
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.

None yet

2 participants