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

Develop: Rebase for OCP resource cleanup on AWS #137

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jyejare
Copy link
Collaborator

@jyejare jyejare commented Aug 1, 2024

Rebasing #134 with Develop branch.

@jyejare jyejare self-assigned this Aug 1, 2024
@jyejare jyejare force-pushed the ocp_cluster_alignment branch 3 times, most recently from 30d9792 to efbdc4f Compare August 3, 2024 13:28
cloudwash/providers/aws.py Show resolved Hide resolved
awscleanup.ocps.cleanup()
if is_dry_run:
echo_dry(dry_data)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might not want to use else since the cleanup can be set to execute OCPs and other resources as well (or use kwargs["_all"])

Copy link
Collaborator Author

@jyejare jyejare Oct 22, 2024

Choose a reason for hiding this comment

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

Do you see a scenario where OCPs and other resources in the list would be called at the same time for cleanup. ?

If yes, we should rather say ocp-clusters to be more specific, so it would look like a resource type within AWS provider.

I thought it is good to keep OCP cluster cleanup separate from other resources cleanup as cluster cleanup will anyways cleanup what it suppose to.

Thoughts ?

cloudwash/entities/resources/ocps.py Outdated Show resolved Hide resolved
Comment on lines +53 to +55
if is_dry_run:
echo_dry(dry_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the last comment, since the cleanup can be executed on multiple resources (OCPs and others), I think the if is_dry_run: component should be outside both sections and appear once only, post all resources cleanup executions.

Suggested change
if is_dry_run:
echo_dry(dry_data)
if is_dry_run:
echo_dry(dry_data)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dry run printing here is per region basis hence having it within a loop makes sense.

If we make it at the end of loop it will only print resources from the last region only as a buggy behavior .

@jyejare
Copy link
Collaborator Author

jyejare commented Aug 8, 2024

Will take a look on the comments tomorrow in brief but did you see any logical failures via static check or execution ?

@oharan2
Copy link
Contributor

oharan2 commented Aug 13, 2024

Will take a look on the comments tomorrow in brief but did you see any logical failures via static check or execution ?

Edit: links in this comment breaked due to force push

Other than the "else" I noted (will break any cleanup contains with both OCPs and other types of AWS resources);

The only thing I suscpected is here, needs to be approachable in any case.

I thought it maybe should be self.dry_data['OCPS']['delete'] when self.dry_data[] will be set in the level of providerclenaup instead of here. Unless it's already verified.

@jyejare
Copy link
Collaborator Author

jyejare commented Oct 22, 2024

@oharan2 We will make the dry run and in general reporting improvements as a next goal. But for now we should be good to go.

Also could you find the time to validate this code on actual cleanup ?

@oharan2
Copy link
Contributor

oharan2 commented Nov 5, 2024

The new create_html command breaks with the OCP outputs.
HERE the function expects an str but the dry data from OCPs are dicts, Logs sent privately.

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.

2 participants