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.

Sign up for GitHub

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

Jump to bottom

CLI: Fix results not being committed in verdi code delete #5848

Closed
wants to merge 3 commits into from
Closed

CLI: Fix results not being committed in verdi code delete #5848

wants to merge 3 commits into from

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 20, 2022

Fixes #5842

The verdi code delete command would report that the Code was deleted
but the changes were actually not committed. This went unnoticed since
the test for it used the click.CliRunner which doesn't follow the
exact same path as a real invocation on the command line. In particular,
in this example the treatment of the session is different with it not
being committed properly in the command line invocation pathway. The
problem is shown by using use_subprocess=True when calling the command
using the run_cli_command fixture, after which the test fails.

@sphuber sphuber requested a review from chrisjsewell December 20, 2022 11:10
@sphuber
Copy link
Contributor Author

sphuber commented Dec 20, 2022

@chrisjsewell the failure started after the changes to the PsqlDosBackend.transaction released in v2.2.0. Most likely verdi code delete is not the only command that is now not properly committing changes.

I am not exactly sure why the changes are not being committed. When the command calls backend.delete_nodes_and_connections it calls backend.transaction and then performs the deletion. The transaction method says the session is already in a transaction so it calls session.begin_nested and returns the session. Clearly the changes are not being committed. This is supposed to be done when the outer transaction (started when session.begin was called for the first time) exits the context manager. I am not sure yet where in the code this is being called and why the commit is not going through.

For now, to demonstrate a potential fix, I added the opening of a transaction in the with_dbenv which should now guarantee that changes performed in CLI commands are committed, but not yet sure that this is the correct approach.

@ltalirz
Copy link
Member

ltalirz commented Dec 20, 2022 •
edited
Loading

Indeed, it appears verdi node delete is broken entirely in 2.2.0
We should either apply a quick fix or roll back the change that was made

@sphuber
Copy link
Contributor Author

sphuber commented Dec 20, 2022

We should either apply a quick fix or roll back the change that was made

The change that caused this fixed another problem related to the same code 😅 so I would prefer not to revert it.

@sphuber sphuber mentioned this pull request Dec 20, 2022
Merged
@sphuber
Fixtures: Add argument use_subprocess to run_cli_command
60e4e30 
By default the `run_cli_command` fixture will run the provided command
through the `CliRunner` utility provided by `click`. However, this
suffers from a problem as the code pathway followed through the runner
is not identical as when the command is actually called through the CLI.

The reason is that in `click` commands do not have a reference to their
parents, and so when a subcommand is invoked through the runner, only
the code of the subcommand is executed. Any code that is part of its
parents, such as parameter validation and callbacks are skipped. In
addition, the runner is executed in the same interpreter as where the
test suite is being run, whereas a command invoked through the actual
CLI runs in a standalone interpreter. This may hide subtle bugs.

Therefore, the `use_subprocess` argument is added, which is `False` by
default, which when set to `True`, the command will not be called through
the runner, but as an actual subprocess using `subprocess.run`. This
guarantees that the tested pathway is identical to what an actual
invocation through the command line would be. It is not enabled by
default since running through a subprocess is slower and not always
necessary.
@sphuber
CLI: Fix results not being committed in verdi code delete
9269952 
The `verdi code delete` command would report that the `Code` was deleted
but the changes were actually not committed. This went unnoticed since
the test for it used the `click.CliRunner` which doesn't follow the
exact same path as a real invocation on the command line. In particular,
in this example the treatment of the session is different with it not
being committed properly in the command line invocation pathway. The
problem is shown by using `use_subprocess=True` when calling the command
using the `run_cli_command` fixture, after which the test fails.
@sphuber sphuber force-pushed the fix/5842/verdi-code-delete branch 2 times, most recently from 125930d to f2ea104 Compare December 20, 2022 13:04
@sphuber sphuber force-pushed the fix/5842/verdi-code-delete branch from f2ea104 to f6870cd Compare December 20, 2022 13:47
@sphuber
Copy link
Contributor Author

sphuber commented Dec 22, 2022

The bug introduced actually affects way more than just verdi code delete. All verdi commands that change state are affected. The fix in this PR is therefore not correct.

@sphuber sphuber closed this Dec 22, 2022
@sphuber sphuber deleted the fix/5842/verdi-code-delete branch December 22, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisjsewell chrisjsewell Awaiting requested review from chrisjsewell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

verdi code delete falsely claims it actually deleted a node
2 participants
@sphuber @ltalirz