Skip to content

Added kuadrant mark to modified test with generation.#698

Merged
azgabur merged 1 commit intoKuadrant:mainfrom
crstrn13:fix_anon_iden
Jul 3, 2025
Merged

Added kuadrant mark to modified test with generation.#698
azgabur merged 1 commit intoKuadrant:mainfrom
crstrn13:fix_anon_iden

Conversation

@crstrn13
Copy link
Copy Markdown
Contributor

Description

Added kuadrant mark to fix failing test on AuthConfig.

Copy link
Copy Markdown
Member

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

Hmm, not sure if this is the best resolution. Yes it will stop the error but we will loose coverage on authorino standalone testing. Especially if the problem is not really in the use case. Could you maybe revise the way the generation field is checked in the wait_for_ready instead? Maybe overriding the method in AuthConfig class which is only CR that doesnt have generation field?

@crstrn13 crstrn13 requested a review from azgabur June 30, 2025 11:53
Copy link
Copy Markdown
Member

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

Im thinking how to do this better way. We would like to avoid using isinstance() in tests but I think this needs maybe bigger backend change and better OOP design of this wait for generation increment. I always thought this wait should be internal to the wait_for_ready method and which should not require any parameter.

We know that AuthConfig does not have generation is status so its impossible to wait for it.

What if all changes via modify_and_apply on Policy class and subclasses would update a state with tells if changes have been pushed to cluster. Something like the .committed property of KubernetesObject. That could be used by wait_for_ready() function to see if there has been a change since first creation and therefore it would wait for observedGeneration

Comment thread testsuite/kuadrant/policy/authorization/auth_config.py Outdated
Copy link
Copy Markdown
Member

@azgabur azgabur left a comment

Choose a reason for hiding this comment

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

I ran make test and make authorino-standalone and all tests passed with this modification.

Comment thread testsuite/kuadrant/policy/__init__.py Outdated
@crstrn13 crstrn13 force-pushed the fix_anon_iden branch 5 times, most recently from f6a15c8 to 0bc8228 Compare July 3, 2025 17:15
Signed-off-by: Alexander Cristurean <acristur@redhat.com>
@azgabur azgabur merged commit 5dbc129 into Kuadrant:main Jul 3, 2025
3 checks passed
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