Skip to content

Conversation

pkoprda
Copy link
Contributor

@pkoprda pkoprda commented Aug 26, 2025

  • Card ID: CCT-994

The test for updating info in the archive was failing in containers, which are not in charge of their own hostnames. This commit resolves the test failures by simulating hostname updates through archive file modification.


This pull request should be also backported to following maintenance branches:

  • el9 (all of RHEL 9)
  • el8 (all of RHEL 8)

@pkoprda pkoprda force-pushed the pkoprda/container-test-failure branch 2 times, most recently from 5cf8c72 to 9192c46 Compare August 26, 2025 16:22
Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

I'm not sure it is a good idea to have a test do wildly different things when running on a host/container. Unless we test it elsewhere already, I'd drop the code changing hostnames and only go with the modified archive.

@pkoprda pkoprda force-pushed the pkoprda/container-test-failure branch 2 times, most recently from 2d5ca03 to 5e646a5 Compare August 27, 2025 13:56
@pkoprda pkoprda changed the title fix(test): Make hostname test work in container environments feat: Add archive modification workflow test Aug 27, 2025
@pkoprda
Copy link
Contributor Author

pkoprda commented Aug 27, 2025

Unless we test it elsewhere already, I'd drop the code changing hostnames and only go with the modified archive.

I kept the original test as was before (I just dropped unnecessary sleep from the test, see the reason in PR description) and I added a new test test_file_workflow_archive_modification_and_upload which is testing modification of the archive and then uploading to Ingress. However, there are already some tests for that in test_upload.py, so I am not sure if this new test is necessary.

@pkoprda pkoprda marked this pull request as ready for review August 27, 2025 14:10
@pkoprda pkoprda force-pushed the pkoprda/container-test-failure branch from 5e646a5 to 371884c Compare August 28, 2025 08:58
Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

If the whole task of this new test is to verify that modified archive gets reflected in Inventory, it 1/ feels like a test Inventory should have, not insights-client, and 2/ we already do that in test_tags.py, just for a different files.

Also, this patch now doesn't address the underlying problem of test_file_workflow_archive_update_host_infofailing because it is callinghostnamectl set-hostname` which does not work in containers.

@pkoprda pkoprda force-pushed the pkoprda/container-test-failure branch from 371884c to 86951b4 Compare September 5, 2025 07:23
@pkoprda pkoprda changed the title feat: Add archive modification workflow test fix: Fix file workflow test for container environments Sep 5, 2025
@pkoprda pkoprda force-pushed the pkoprda/container-test-failure branch from 86951b4 to c794413 Compare September 5, 2025 09:53
Copy link
Contributor

@m-horky m-horky left a comment

Choose a reason for hiding this comment

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

All things considered, LGTM. I'm not too happy insights-client does this functionality through the Core, and it might be better to test this through the Core collection, but since it also is a question of integration, we might as well do it.

@m-horky m-horky requested a review from zpetrace September 8, 2025 09:57
* Card ID: CCT-994

Rename remove_files_from_archive() to modify_archive(), which adds
support for adding files in addition to removing them.
* Card ID: CCT-994

The test for updating info in the archive was failing in containers,
which are not in charge of their own hostnames. This commit resolves the
test failures by simulating hostname updates through archive file
modification.
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.

2 participants