Skip to content

Comments

Add IOP#280

Open
ehelms wants to merge 13 commits intotheforeman:masterfrom
ehelms:add-iop
Open

Add IOP#280
ehelms wants to merge 13 commits intotheforeman:masterfrom
ehelms:add-iop

Conversation

@ehelms
Copy link
Member

@ehelms ehelms commented Nov 2, 2025

No description provided.

@ehelms ehelms force-pushed the add-iop branch 5 times, most recently from bfdcd43 to 2fd7276 Compare November 4, 2025 01:51
@ehelms ehelms marked this pull request as ready for review November 4, 2025 16:45
@ehelms ehelms force-pushed the add-iop branch 2 times, most recently from 71ce03e to 4cf96ac Compare November 25, 2025 20:35
@ehelms ehelms force-pushed the add-iop branch 6 times, most recently from ddfec69 to eb1df00 Compare December 11, 2025 20:38
@ehelms
Copy link
Member Author

ehelms commented Dec 12, 2025

@evgeni Could you check just the github action part of the commit (eb1df00) to make sure it matches with how you think we should use matrix?

Comment on lines +114 to +117
- name: Enable iop
if: matrix.iop == 'enabled'
run: |
./foremanctl deploy --add-feature iop
Copy link
Member

Choose a reason for hiding this comment

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

calls to foremanctl deploy are expensive (adds ca 2 minutes to the execution), so I wonder if it'd be smarter to have fewer of those "add optional feature" steps and use something like {{ matrix.iop == 'enabled' && '--add-feature iop' || '' }} inside an existing step

@evgeni
Copy link
Member

evgeni commented Dec 12, 2025

@evgeni Could you check just the github action part of the commit (eb1df00) to make sure it matches with how you think we should use matrix?

I think it can be optimized, posted comments how :)

@ehelms ehelms force-pushed the add-iop branch 5 times, most recently from a200e68 to ffcfc02 Compare December 15, 2025 16:58
@ehelms
Copy link
Member Author

ehelms commented Dec 15, 2025

@evgeni For now, I want to keep the same behavior of not supporting remote database yet. Looking at Obsah, I don't think that's possible but I wanted you to check me on that. I need to forbid:

  • database_mode == external and 'iop' in enabled_features

forbidden_if looks like it only allows the first parameter to have a value. Do I read that correctly?

@ehelms ehelms force-pushed the add-iop branch 2 times, most recently from d5bfe6f to 1b2bf3f Compare December 17, 2025 21:29
@evgeni
Copy link
Member

evgeni commented Dec 18, 2025

@evgeni For now, I want to keep the same behavior of not supporting remote database yet. Looking at Obsah, I don't think that's possible but I wanted you to check me on that. I need to forbid:

* database_mode == external and 'iop' in enabled_features

forbidden_if looks like it only allows the first parameter to have a value. Do I read that correctly?

That's correct, today we can only forbid "if param P has value Y you can't set params A and B at all"

You could add a check, that's more flexible than what you get at the obsah level?

@ehelms
Copy link
Member Author

ehelms commented Dec 18, 2025

You could add a check, that's more flexible than what you get at the obsah level?

I could... just less clear then when do we do parameter validation at the CLI level and when do we do it at the checks level.

@ehelms
Copy link
Member Author

ehelms commented Dec 18, 2025

Here is my attempt to implement that structure: theforeman/obsah#104

@ehelms ehelms force-pushed the add-iop branch 3 times, most recently from 95c8203 to acbe14a Compare December 18, 2025 19:44
Copy link
Contributor

@pablomh pablomh left a comment

Choose a reason for hiding this comment

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

Builds and creates running services :)

@ehelms ehelms changed the title Add core services for iop Add IOP Jan 20, 2026
@ehelms ehelms force-pushed the add-iop branch 2 times, most recently from e96e49f to c333fd0 Compare February 2, 2026 13:58
@ehelms
Copy link
Member Author

ehelms commented Feb 3, 2026

I'll need theforeman/puppet-certs#505 to get in so that installer certificate testing can pass.

@ehelms ehelms force-pushed the add-iop branch 3 times, most recently from 85f7f48 to 2e42420 Compare February 4, 2026 13:24
Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>
Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>
Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>
Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>
@@ -0,0 +1,8 @@
---
iop_engine_container_image: "quay.io/iop/insights-engine"
iop_engine_container_tag: "foreman-3.16"
Copy link

@vkrizan vkrizan Feb 19, 2026

Choose a reason for hiding this comment

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

Is there any reason to split image and its tag? Doing so this split would prevent anyone using digest only? Although with a combination with a tag the digest can still be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does it prevent using digest only?

IIRC the reasoning was to make it easier to do what we do in the vars file: https://github.com/theforeman/foremanctl/blob/master/src/vars/images.yml#L1C1-L1C21

We can set the current tag for all images without having to touch the registry information and vice versa.

Copy link

Choose a reason for hiding this comment

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

In that case feel free to keep it as is.

The digest only could look like

quay.io/iop/insights-engine@sha256:7a777a4ff121d259c9e592ce2425006631ecdaa7f3b001b184ede049b2a7a7a9

with the split and format it would end up as invalid reference format because of that colon

quay.io/iop/insights-engine:@sha256:7a777a4ff121d259c9e592ce2425006631ecdaa7f3b001b184ede049b2a7a7a9
                           ^

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see. That's a fair call out, will you make a Github issue with that? I don't want to change the paradigm already present in the repository with this PR but I agree we should explore that.

Copy link

Choose a reason for hiding this comment

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

Sure!



def pytest_runtest_setup(item):
if "iop" in item.nodeid.lower():
Copy link

Choose a reason for hiding this comment

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

Pytest has markers that could be utilized instead of a substring match of a test name which is rigid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what that means or would entail, but I'll look it up.

Copy link

Choose a reason for hiding this comment

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

assert "running" in result.stdout


def test_kafka_quadlet_file(server):
Copy link

Choose a reason for hiding this comment

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

Why are these two tests being removed in this commit (and overall)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which commit? The interface doesn't actually show me.

Copy link

Choose a reason for hiding this comment

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

cf46b19
Add iop_ingress

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.

4 participants