Skip to content

fix(tests): pre-create the testing namespaces for cross-namespace resource references tests #113

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

Merged

Conversation

michaelhtm
Copy link
Member

@michaelhtm michaelhtm commented Jul 30, 2024

Description of changes:
Making changes to previous PR, to make sure namespaces are being tested

@pytest.fixtures(scope="module") don't pick up custom @pytest.mark so we changed the scope to "function"

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested review from a-hilaly and jaypipes July 30, 2024 23:17
@michaelhtm michaelhtm force-pushed the namespace-reference/test branch 11 times, most recently from a47f765 to f41bc98 Compare July 31, 2024 17:39
Comment on lines 205 to 209
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all these?

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 will be testing with just one wait


# create the resources in order that initially the reference resolution
# fails and then when the referenced resource gets created, then all
# resolutions eventually pass and resources get synced.
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

are you sure we don't need this anymore?

Comment on lines 113 to 115
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
time.sleep(CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to sleep 3 times here?

if marker is not None:
data = marker.args[0]
if 'withNamespace' in data:
if 'withNamespace' in data and data['withNamespace']:
logging.info("ChangingNamespaceInPolicy")
Copy link
Member

Choose a reason for hiding this comment

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

logging not needed

Comment on lines 110 to 114
k8s.create_k8s_namespace(
TESTING_NAMESPACE
)
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to create the namespace at this level? why not at the beginning of the test function?

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 want to only create the namespace if needed..I will get namespace already exists error if it's created for the second test.
Also referred_policy runs before the test does

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, thank you!

@@ -34,31 +34,29 @@
# Little longer to delete the policy since it's referred-to from the role...
DELETE_POLICY_TIMEOUT_SECONDS = 30
CHECK_WAIT_AFTER_REF_RESOLVE_SECONDS = 10
TESTING_NAMESPACE = "custom_namespace"
TESTING_NAMESPACE = random_suffix_name("policy-namespace", 24)
Copy link
Member

Choose a reason for hiding this comment

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

can we do something similar to L40?

@pytest.fixture(scope="module")
def referred_policy_name():

.gitignore Outdated
build
/deploymentFiles
Copy link
Member

Choose a reason for hiding this comment

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

are these specific to ACK

Copy link
Member Author

Choose a reason for hiding this comment

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

no that's just a directory i created, will remove

@@ -189,19 +191,22 @@ def test_role_policy_references(self, referring_role, referred_policy):
role.wait_until_deleted(role_name)

@pytest.mark.resource_data({'withNamespace': True})
def test_role_policy_namespace_references(self, referring_role, referred_policy):
def test_role_policy_namespace_references(self, referring_role, referred_policy, request):
Copy link
Member

Choose a reason for hiding this comment

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

is request needed in the parameters?

@michaelhtm michaelhtm force-pushed the namespace-reference/test branch from f41bc98 to 2777032 Compare July 31, 2024 18:28
@michaelhtm michaelhtm changed the title Namespace reference/test FIX: testing namespace cross reference Jul 31, 2024
@michaelhtm michaelhtm changed the title FIX: testing namespace cross reference fix(tests): pre-create the testing namespaces for cross-namespace resource references tests Jul 31, 2024
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Sweet, thank you for the fixes!
/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2024
Copy link

ack-prow bot commented Jul 31, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot added the approved label Jul 31, 2024
@ack-prow ack-prow bot merged commit 909a305 into aws-controllers-k8s:main Jul 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants