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

Utilize atexit making sure connection info is deleted #2352

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

oysteoh
Copy link
Contributor

@oysteoh oysteoh commented Nov 12, 2021

No description provided.

@oysteoh oysteoh requested a review from pinkwah November 12, 2021 08:09
@oysteoh oysteoh force-pushed the remove_connection_info branch 3 times, most recently from 4c76b2c to f158799 Compare November 12, 2021 08:29
@oysteoh
Copy link
Contributor Author

oysteoh commented Nov 12, 2021

Jenkins: Test this please!

"""
file = Path(f"{service_name}_server.json")
if file.exists():
file.unlink()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like to unregister?:

Suggested change
file.unlink()
file.unlink()
atexit.unregister(delete_connection_info)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nop, no difference

@oysteoh oysteoh force-pushed the remove_connection_info branch 4 times, most recently from a66d11b to 7c030fd Compare November 12, 2021 14:16
@atexit.register
def cleanup_service_files():
for service_name in SERVICE_NAMES:
file = Path(f"{service_name}_server.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

this says "server", while the test says "service"

@oysteoh oysteoh force-pushed the remove_connection_info branch 2 times, most recently from 7f43a57 to f8452ac Compare November 18, 2021 07:51

def test_cleanup_service_files(tmpdir):
global SERVICE_NAMES

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this global should be deleted or cleaned up so it does not mess up other tests?

Copy link
Contributor

@frode-aarstad frode-aarstad left a comment

Choose a reason for hiding this comment

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

LGTM

@oysteoh oysteoh enabled auto-merge (rebase) November 18, 2021 12:10
@oysteoh oysteoh merged commit e2a8371 into equinor:main Nov 18, 2021
@oysteoh oysteoh deleted the remove_connection_info branch April 20, 2022 06:31
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.

None yet

4 participants