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

Deprecate addgroup #307

Merged
merged 31 commits into from
Feb 26, 2024
Merged

Deprecate addgroup #307

merged 31 commits into from
Feb 26, 2024

Conversation

maouw
Copy link
Contributor

@maouw maouw commented Jan 31, 2024

Builds on #306 to fix failing tests:


I got the tests to succeed using moto[cloudformation]==4.1.5.

It started succeeding with moto==4.1.10. I chose this because 4.1.11 is where there were some changes to handling duplicate tags, according to the changelog:

  • ECR: put_image(): now behaves correctly on duplicate images with duplicate tags

When I first tried with moto==4.1.10, test_DockerImage succeeded. However, test_pars_with_new_vpc started to fail at cloudknot/tests/test_pars.py:298:

An error occurred (DependencyViolation) when calling the DeleteStack operation: The vpc vpc-b2ae771a has dependencies and cannot be deleted.

I went back to the moto changelog, and looked for where there were any potential changes to CloudFormation, and found 4.1.6 changed how stack deletion was handled:

  • CloudFormation now supports deletion of AWS::EC2::Subnet, AWS::EC2::VPC
  • CloudFormation now supports variable mapping inside "Fn::Sub"
  • CloudFormation: delete_stack() now adheres to "DeletionPolicy": "Retain" set for individual resources

So, I set moto==4.1.5 and it worked!

There is one warning that we might want to check out:

cloudknot/tests/test_knot.py::test_knot
  . . . moto/ec2/models/instances.py:140: PendingDeprecationWarning: Could not find AMI with image-id:None, in the near future this will cause an error.
  Use ec2_backend.describe_images() to find suitable image for your test

Originally posted by @maouw in #306 (comment)

arokem and others added 29 commits July 29, 2023 10:50
And also add testing on newer 3.10.
@maouw maouw marked this pull request as draft January 31, 2024 21:41
@maouw maouw marked this pull request as ready for review January 31, 2024 22:21
@coveralls
Copy link

coveralls commented Feb 25, 2024

Coverage Status

coverage: 0.0%. remained the same
when pulling ef81704 on maouw:deprecate_addgroup
into 33c73bf on nrdg:master.

Copy link
Member

@arokem arokem left a comment

Choose a reason for hiding this comment

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

OK - looks great! I am suggesting to also upgrade our supported Python versions to 3.10 and 3.11, based on https://scientific-python.org/specs/spec-0000/

.github/workflows/docbuild.yml Outdated Show resolved Hide resolved
.github/workflows/pythonpackage.yml Outdated Show resolved Hide resolved
@arokem
Copy link
Member

arokem commented Feb 26, 2024

Committed my changes, so we can get the CI to run with those changes and merge this if it all works.

@arokem arokem merged commit 7c8b442 into nrdg:master Feb 26, 2024
4 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.

3 participants