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

Update configurations after clean #68

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

Navarro-Jonathan
Copy link
Collaborator

@Navarro-Jonathan Navarro-Jonathan commented Aug 9, 2023

PersistentConfigurationManager will read the persistent files only once for each instance of the program. This is not usually a problem when using the CLI. However, if a user were to call clean through the API, PersistentConfigurationManager's cached configurations would not be updated. This means that subsequent calls using a CLI runner to configurations list, or any API functions would not read the change to the persistent configurations.

Note that this is not a problem in the other functions that alter the persistent files, since they change PersistentConfigurationManager's cached configurations accordingly.

To resolve this, clean now makes PersistentConfigurationManager re-read the persistent files after deleting them. Additionally, the active session is now unset.

@Navarro-Jonathan Navarro-Jonathan self-assigned this Aug 9, 2023
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:31 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:31 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:31 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:31 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan marked this pull request as draft August 9, 2023 21:39
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:42 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:42 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:42 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 9, 2023 21:42 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan changed the title Update PersistentConfigurationManager after clean Update configurations after clean Aug 9, 2023
@Navarro-Jonathan Navarro-Jonathan marked this pull request as ready for review August 9, 2023 21:51
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 18:37 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 18:37 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 18:37 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 18:37 — with GitHub Actions Inactive
@cartermak
Copy link
Member

Could we do a similar reset method for the persistent sessions? Something that just runs cls._active_session = None.

@Navarro-Jonathan
Copy link
Collaborator Author

Navarro-Jonathan commented Aug 10, 2023

Could we do a similar reset method for the persistent sessions? Something that just runs cls._active_session = None.

In unset_active_session, the .session files are also being deleted:
https://github.com/NASA-AMMOS/aerie-cli/blob/5b0d373877b1fc76d42b73f7dbff0ea33eb38833/src/aerie_cli/persistent.py#L177-L197C20

However, we don't need to read the name from the session, and we could remove the call to _load_active_session.

So we could create a reset with something along these lines:

cls._active_session = None

# Get any/all open sessions. List in chronological order, newest first
fs: List[Path] = [
    f for f in SESSION_FILE_DIRECTORY.glob('*.aerie_cli.session')]
if not len(fs):
    return
# Delete all session files
for fn in fs:
    fn.unlink()

@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 22:17 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 22:17 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 22:17 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 10, 2023 22:17 — with GitHub Actions Inactive
@cartermak
Copy link
Member

What's the behavior of SESSION_FILE_DIRECTORY.glob('*.aerie_cli.session') when that directory has been deleted, though? Will that line error out?

@Navarro-Jonathan
Copy link
Collaborator Author

What's the behavior of SESSION_FILE_DIRECTORY.glob('*.aerie_cli.session') when that directory has been deleted, though? Will that line error out?

It does not seem to throw any errors even if that directory has been deleted. It just continues silently.

@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan temporarily deployed to integration-test-workflow August 15, 2023 20:51 — with GitHub Actions Inactive
@Navarro-Jonathan Navarro-Jonathan merged commit 80159ed into develop Aug 15, 2023
12 checks passed
@Navarro-Jonathan Navarro-Jonathan deleted the persistent-configuration-fix branch August 15, 2023 20:54
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.

2 participants