Skip to content
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

added force_destroy to dns module #2202

Merged
merged 4 commits into from
Apr 5, 2024
Merged

Conversation

nika-pr
Copy link
Contributor

@nika-pr nika-pr commented Apr 4, 2024

force_destroy was missing from the DNS module, so I added it.


Checklist

If applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass:

Outdated:
When I execute pytest -k 'modules and dns:' tests/examples,
I am getting pytest: error: unrecognized arguments: --dist.
I am using Python 3.10.12 and pytest 8.1.1 on WSL.

Copy link

google-cla bot commented Apr 4, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@juliocc juliocc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @nika-pr!

Left one small comment below

modules/dns/variables.tf Outdated Show resolved Hide resolved
@juliocc juliocc enabled auto-merge (squash) April 4, 2024 16:15
@wiktorn
Copy link
Collaborator

wiktorn commented Apr 4, 2024

  • When I execute pytest -k 'modules and dns:' tests/examples,
    I am getting pytest: error: unrecognized arguments: --dist.
    I am using Python 3.10.12 and pytest 8.1.1 on WSL.

Regarding this error. This could happen if you're missing pytest-xdist package installed, but it is in tests/requirements.txt for quite a long time.

Does installing pytest-xdist manually solve the issue for you?

@juliocc
Copy link
Collaborator

juliocc commented Apr 4, 2024

but it is in tests/requirements.txt for quite a long time

It's not ;)

AFAIK you don't need pytest-xdist unless you're passing -n to pytest

@wiktorn
Copy link
Collaborator

wiktorn commented Apr 4, 2024

but it is in tests/requirements.txt for quite a long time

It's not ;)

Is it? ;-)

AFAIK you don't need pytest-xdist unless you're passing -n to pytest

You needed, because I've added pytest.ini entry, which depends on having pytest-xdist installed always.

@ludoo
Copy link
Collaborator

ludoo commented Apr 4, 2024

You needed, because I've added pytest.ini entry, which depends on having pytest-xdist installed always.

hmmmm do we really need it?

edit: I don't much like forcing pytest behaviour on users...

@wiktorn
Copy link
Collaborator

wiktorn commented Apr 4, 2024

You needed, because I've added pytest.ini entry, which depends on having pytest-xdist installed always.

hmmmm do we really need it?
edit: I don't much like forcing pytest behaviour on users...

We can remove that, but then users running e2e tests needs to add --dist loadgroup so tests that can't run in parallel, are executed one after another.

As running E2E is niche as of now, this can be removed, but i prefer stuff to just work by default.

@nika-pr
Copy link
Contributor Author

nika-pr commented Apr 5, 2024

Does installing pytest-xdist manually solve the issue for you?

Yes it did, alls DNS module tests passed :)

auto-merge was automatically disabled April 5, 2024 09:03

Head branch was pushed to by a user without write access

@ludoo ludoo enabled auto-merge (squash) April 5, 2024 09:10
@ludoo ludoo merged commit 0cae2ff into GoogleCloudPlatform:master Apr 5, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants