Skip to content

Conversation

@efussi
Copy link

@efussi efussi commented Apr 9, 2025

SUMMARY

Don't set DeleteOptions in k8s when running in check mode.

Fixes #892

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

k8s

ADDITIONAL INFORMATION

See ACTUAL and EXPECTED RESULTS in #892 (comment) for command output before and after the change.

@softwarefactory-project-zuul
Copy link

Copy link
Contributor

@abikouo abikouo left a comment

Choose a reason for hiding this comment

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

You need to add a changelog with something like

---
bugfixes:
  - module_utils/k8s/service - fix issue when trying to delete resource using `delete_options` and `check_mode=true` (https://github.com/ansible-collections/kubernetes.core/issues/892).

It would be nice if you can also update the k8s_delete integration tests target to validate the fix

@softwarefactory-project-zuul
Copy link

@efussi
Copy link
Author

efussi commented Apr 10, 2025

Added the changelog fragment and will look at adding an integration test next.

@efussi
Copy link
Author

efussi commented Apr 11, 2025

Hint to self: make test-integration only works when the repo is cloned into a particular directory structure, see https://docs.ansible.com/ansible/latest/community/collection_contributors/collection_test_pr_locally.html#how-to-test-a-collection-pr. Next step is to make the existing integration tests pass, currently they all fail.

@efussi
Copy link
Author

efussi commented Apr 13, 2025

Notes from setting up the environment to run the integration tests:

  • The integration tests modify the cluster pointed at by the current kubeconfig (!!!)
  • The Makefile variable to set integration test arguments has an unusual leading ?: make test-integration ?TEST_ARGS=k8s_delete (not sure if $(?TEST_ARGS) has a special meaning in non-GNU variants of make, in my version of make the ? is considered part of the variable name which doesn't match TEST_ARGS ?= "" at the beginning of the Makefile)
  • The nginx-d pod started by the k8s_delete test target needs to run as root, this breaks on OpenShift when the default restricted-v2 SCC is used. Prepare the cluster like this:
    oc new-project delete  # ensure integration test namespace `delete` exists
    oc adm policy add-scc-to-user anyuid -z default  # bind the `default` service account to the `anyuid` SCC
    

@efussi efussi requested a review from abikouo April 13, 2025 19:07
@efussi
Copy link
Author

efussi commented Apr 13, 2025

@abikouo I added an integration test. Can you please let me know if any further changes are needed?

@softwarefactory-project-zuul
Copy link

@efussi
Copy link
Author

efussi commented Apr 24, 2025

@abikouo This PR is ready for review now. There are some failing tests, but I think that's unrelated to this PR (at least I have no idea what I could do about this):

=========================== short test summary info ============================
FAILED tests/unit/module_utils/test_core.py::test_warn_on_k8s_version[stdin0] - AssertionError: assert 'kubernetes' in {'__ansible_type': 'WarningSummary', 'details': [{'__ansible_type': 'Detail', 'msg': 'kubernetes<24.2.0 is not supported or tested. Some features may not work.'}]}
======================== 1 failed, 185 passed in 15.12s ========================

@yurnov
Copy link
Contributor

yurnov commented Apr 24, 2025

@abikouo This PR is ready for review now. There are some failing tests, but I think that's unrelated to this PR (at least I have no ide what I could do about this):

=========================== short test summary info ============================
FAILED tests/unit/module_utils/test_core.py::test_warn_on_k8s_version[stdin0] - AssertionError: assert 'kubernetes' in {'__ansible_type': 'WarningSummary', 'details': [{'__ansible_type': 'Detail', 'msg': 'kubernetes<24.2.0 is not supported or tested. Some features may not work.'}]}
======================== 1 failed, 185 passed in 15.12s ========================

It's an issue with the test for the current devel and milestone branches of ansible. Here is issue #904 and a prepared fix #903

@yurnov
Copy link
Contributor

yurnov commented Apr 24, 2025

Hi @efussi

Would you please rebase your PR or merge main into? It should resolve a issue with failing test

@softwarefactory-project-zuul
Copy link

@abikouo abikouo mentioned this pull request Apr 24, 2025
@abikouo
Copy link
Contributor

abikouo commented Apr 24, 2025

Replaced by #905
CC @yurnov @beeankha

@abikouo abikouo closed this Apr 24, 2025
@efussi
Copy link
Author

efussi commented Apr 24, 2025

Hi @efussi

Would you please rebase your PR or merge main into? It should resolve a issue with failing test

@abikouo beat me to it 😄. Thanks for taking care of this!

softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 25, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
patchback bot pushed a commit that referenced this pull request Apr 25, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
(cherry picked from commit d329e7e)
patchback bot pushed a commit that referenced this pull request Apr 25, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
(cherry picked from commit d329e7e)
patchback bot pushed a commit that referenced this pull request Apr 25, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
(cherry picked from commit d329e7e)
patchback bot pushed a commit that referenced this pull request Apr 25, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
(cherry picked from commit d329e7e)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 25, 2025
This is a backport of PR #905 as merged into main (d329e7e).
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Bikouo Aubin
Reviewed-by: Bianca Henderson <[email protected]>
abikouo added a commit that referenced this pull request May 2, 2025
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Yuriy Novostavskiy
Reviewed-by: Bianca Henderson <[email protected]>
(cherry picked from commit d329e7e)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request May 2, 2025
This is a backport of PR #905 as merged into main (d329e7e).
This PR is a rebase of #898 for CI to pass
Thanks @efussi for your collaboration.
Closes #892

Reviewed-by: Bianca Henderson <[email protected]>
Reviewed-by: Bikouo Aubin
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.

kubernetes.core.k8s deletes objects in check mode if delete_options are set

4 participants