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

ref: Migrate object_cluster_list module to ListModule base class; add deprecation support to ListModule and InfoModule #575

Merged

Conversation

lgarber-akamai
Copy link
Contributor

@lgarber-akamai lgarber-akamai commented Aug 22, 2024

📝 Description

This pull request migrates the object_cluster_list module over to the ListModule base class. Because this module is deprecated, this PR also adds support for deprecating modules using the ListModule and InfoModule base classes.

NOTE: object_cluster_info was intentionally not migrated as a part of this PR because its usage deviates too much from InfoModule.

✔️ How to Test

The following test steps assume you have pulled down this PR locally and run make install.

Integration Testing

  1. Open tests/integration/targets/object_cluster_list/main.yaml, remove the debug task and uncomment the rest of the playbook.
  2. Run the tests:
make TEST_ARGS="-v object_basic object_cluster_list" test

@lgarber-akamai lgarber-akamai added the improvement for improvements in existing functionality in the changelog. label Aug 22, 2024
@@ -10,18 +10,6 @@
# - assert:
# that:
# - no_filter.clusters | length >= 1
#
# - name: List regions with filter on region
Copy link
Contributor Author

@lgarber-akamai lgarber-akamai Aug 22, 2024

Choose a reason for hiding this comment

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

This no longer filters as expected in both the old and new module implementations. Given this is a deprecated API, do we think it's worth keeping and investigating further?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can skip and save effort since it's deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

@lgarber-akamai lgarber-akamai marked this pull request as ready for review August 22, 2024 18:43
@lgarber-akamai lgarber-akamai requested a review from a team as a code owner August 22, 2024 18:43
@lgarber-akamai lgarber-akamai requested review from jriddle-linode and yec-akamai and removed request for a team August 22, 2024 18:43
Copy link
Contributor

@yec-akamai yec-akamai left a comment

Choose a reason for hiding this comment

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

Works as expected!

@@ -10,18 +10,6 @@
# - assert:
# that:
# - no_filter.clusters | length >= 1
#
# - name: List regions with filter on region
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can skip and save effort since it's deprecated.

Copy link
Collaborator

@jriddle-linode jriddle-linode left a comment

Choose a reason for hiding this comment

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

Looks much better and works locally.

@lgarber-akamai lgarber-akamai merged commit e5c9ea4 into linode:dev Sep 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement for improvements in existing functionality in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants