aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
411 stars 184 forks source link

Consistent `--profile-name` option for setup commands #6481

Closed GeigerJ2 closed 1 day ago

GeigerJ2 commented 1 week ago

Currently, verdi presto uses the --profile-name option, while verdi profile setup uses --profile. I'd vouch for consistency.

GeigerJ2 commented 1 week ago

Along these lines, I just realized that the options for the database hostname, port, username, and password for verdi presto --use-postgres are called: --postgres-hostname --postgres-port --postgres-username --postgres-password (which makes sense as --use-postgres is explicitly specified, I guess).

While for verdi profile setup core.psql_dos they are called: --database-hostname --database-port --database-username --database-password

Any reason to keep them general in the second case (apart from backwards compatibility?). Are we planning to add another service-based database? Again, might be nice to have the options named consistently, so that people can switch seamlessly between writing a verdi presto and a verdi profile setup command (which is how I noticed now).

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.77%. Comparing base (ef60b66) to head (fa3a47a). Report is 53 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6481 +/- ## ========================================== + Coverage 77.51% 77.77% +0.27% ========================================== Files 560 561 +1 Lines 41444 41782 +338 ========================================== + Hits 32120 32491 +371 + Misses 9324 9291 -33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mbercx commented 1 week ago

Checking the status of the AMD runners, will restart actions after.'

EDIT: Seems to be a more substantial issue.. @unkcpz any ideas?

unkcpz commented 1 week ago

https://github.com/aiidateam/aiida-core/blob/022f049bfcf86609daf0c2d9ddc0b1c108b9ea7c/.docker/aiida-core-base/s6-assets/init/aiida-prepare.sh#L29

Should update this as well.

sphuber commented 2 days ago

I am onboard with creating consistency, but is --profile-name not clearer? I would personally vote for that. We can add a short version -p to counter-act the increased length, which anyway would be a good idea.

GeigerJ2 commented 1 day ago

I am onboard with creating consistency, but is --profile-name not clearer? I would personally vote for that. We can add a short version -p to counter-act the increased length, which anyway would be a good idea.

Alright, that's fine for me, as long as there's also the short option ^^ I added another commit to revert the changes (rather than soft-resetting and force-pushing; to preserve the history), and reverted the files to their previous versions via git checkout <SHA> -- </path/to/file.py> in the first commit. Then, the second commit defines the new SETUP_PROFILE_NAME option. I didn't clone the previous SETUP_PROFILE, as that is only used in the deprecated verdi setup command, so can be removed once we remove that command, as well. I considered using this global SETUP_PROFILE_NAME in the verdi presto command, as well, rather than having it as a local @click.option as it is now, but that slightly changes the behavior, so I wouldn't do that. I assume there was a reason you added it as a local @click.option.

As such, the behavior of both commands stays the same, with verdi profile setup still being an InteractiveOption, while verdi presto still being a normal @click.option. The former now also uses --profile-name, and both allow -p as a shortcut.

Pinging @mbercx as the extra picky devil's advocate on CLI-related topics <3

GeigerJ2 commented 1 day ago

I am onboard with creating consistency, but is --profile-name not clearer? I would personally vote for that. We can add a short version -p to counter-act the increased length, which anyway would be a good idea.

Though, now that I'm changing the command in a bunch of places, I'm also thinking... this is backwards-incompatible with the previous version of verdi profile setup that uses --profile. It's a recent feature that was just introduced in v2.5, so it might be acceptable, but still something to consider? That being said, I'm also very much in favor of using --profile-name rather than --profile, because --profile is also used when running verdi commands for a different profile than the default one via verdi --profile. So good to have it distinctly different from that.

sphuber commented 1 day ago

Though, now that I'm changing the command in a bunch of places, I'm also thinking... this is backwards-incompatible with the previous version of verdi profile setup that uses --profile. It's a recent feature that was just introduced in v2.5, so it might be acceptable, but still something to consider?

I think that is acceptable for now. Better to bite the bullet now while it hasn't really been adopted yet then having to struggle with it later on.

GeigerJ2 commented 1 day ago

I think that is acceptable for now. Better to bite the bullet now while it hasn't really been adopted yet then having to struggle with it later on.

I'm fine with that. @mbercx also mentioned we could deprecate --profile and hide it from being shown from --help (if that's possible), but not sure if it's even necessary (we can discuss quickly later). Apart from that, this should be ready for merge now.

sphuber commented 1 day ago

Thanks @GeigerJ2