Redo package server upload script

[teutoburg]

New upload script

This is a complete revamp of the IRDB publish script used to push packages to the server, in line with the changes made to the download in AstarVienna/ScopeSim#234. The script now used argparse instead of manually parsing command-line arguments. These arguments have been modified and the descriptions updated accordingly. The password can now be entered in a non-echoing way, however passing it as an argument is still supported, mainly intended for automated use. When publishing a new stable version of a package, the user is prompted for confirmation, to avoid unintended uploads. This can be disabled for automated use. Since we're not using the packages.yaml file anymore, the serverside sub-directories for the packages (locations, telescopes and instruments) are now stored in server_folders.yaml. If upload of a package without an entry in that file is attempted, the user is prompted to enter the folder interactively (or abort).

Tests

The test suite has been more or less completely redone, which was necessary for the new structure of the upload script. Multiple combinations of command-line arguments are tested.

Still to do

• The tests do not cover everything. This could be improved.
• Even before this change, some code was duplicated in ScopeSim and here. This has now increased, which is not ideal. This code should be in a separate package (scopesim_core?), that would then be a dependency of both ScopeSim and IRDB.
• The new script is intended to be an executable CLI. This could be achieved by making an actual python package with the irdb core functionality (excluding the instrument data). More on that later...

[teutoburg]

Some tests (5, but actually just 2, rest is parametrized) still fail, I marked them with xfail for now. Everything works fine when I run the tests locally, so it's probably to do with the CI. The tests in question all mock sys.stdout to capture output, so it's most likely related to how the CI deals with that. Anyway, things should most likely work just fine. We can deal with those tests later, the test suite still needs some expansion anyway...

[hugobuddel]

Great! It uses argparse, explicitly deprecates packages.yaml, and is properly tested! So I'm happy. And type annotations, and pathlib.

My main concern is that Password seems to make it easy to accidentally leak a password in some unexpected way. But there is no concrete reason to expect that though, so still approved.

Also 2 comments on how to get the test working.

[hugobuddel]

This test fails because there is no test_package.2023-07-20.zip and no test_package.2023-07-20.dev.zip in _ZIPPED_PACKAGES.

I´d be fine if we just commit those two files to the repository. Or we could create them in the test, or in a fixture, or we just keep the xfail, all fine with me.

[teutoburg]

That's what I attempted to do with the temp_zipfiles fixture. Obviously didn't work for some reason. It did work on my machine, I also don't have these two versions of the test_package in _ZIPPED_PACKAGES. I think the way to go would be to try and make the fixture approach work (at a later point).

[hugobuddel]

Having __str__() defined in a password class doesn´t feel right to me. Maybe this will lead to passwords being printed in some debug log or whatever. Is it only for __eq__? I think it would be better to remove this function, because I can't see how it can lead to something good, but I can see how it leads to something bad. Or am I missing anything?

Suggested change

	def __str__(self):
	return self.value

[hugobuddel]

To make __eq__() work with my suggestion to remove __str__()

Suggested change

	return str(self) == str(other)
	return self.value == other.value

[hugobuddel]

Minor nitpicking, the default of -p requires one to enter a password even if -u is not specified. And -l is also required. So just compiling a package now requires something like python ./irdb/publish.py -c test_package -l Q -p Q. But that's good enough, since we are the only ones using it. And I believe this was already the case in the old version.

[teutoburg]

Comment on the password situation (you had three separate comments above, I'll just put it all here):

Firstly, I'm not totally happy with how it turned out. Mostly because it now shows the prompt every time the thing is executed without -p pwd, which already annoyed me in debugging. Before my modifications, the only way to supply the password was via that argument, which imo was worse, because it would echo in the console every time.

I spent some time on stackoverflow et al. to see how this is done in other projects. The main complication was to allow both command line argument and the more secure prompt. For the latter, getpass seems to be the standard thing. The current implementation with the custom class is modified from some stackoverflow answer. The __str__() is used to actually get the password out of the object to supply it to the function handling the upload. The __eq__() I added to get a test working that checks if the (mock) password was correctly forwarded to the function.

And yes, requiring -l and -p just to -c but not -u a package is not ideal either. That could be an easier fix though.

The most straightforward way to solve most if not all of that would probably be to prompt both username and password (using getpass), but only when -u is set. That would remove the ability to easily run an automated script, uploading a package from the CD. But that might be solvable, just not with my current knowledge 🙃

Either way, I think "merge for now, improve later" is probably the most reasonably approach. This is already a rather large change.

[teutoburg]

Erm, your capfd suggestion is not working either. Any idea what's going on @hugobuddel ? 😅 Feel free to look at it tomorrow though, it's getting late...

As I'll already be on a plane tomorrow morning: If you do manage to fix that test tomorrow (or later), please feel free to commit that fix and merge this! 👍

[hugobuddel]

Hmmz, that capfd thing "did work for me". I'll get this fixed and merge it, you go on vacation.

[hugobuddel]

It works nicely now. The secret to capturing the output was using caplog. capfd only worked because I added -s to my pytest run..

The other xfail-ed test works now too, I just added your temp_zipfiles module to that test too, probably slipped your mind.

I did change the Password class a bit though; simply by using password.value instead of str(password). I noticed there is no test that tests whether the right password is provided, and I don't understand mocking well enough to add one. So I tested it manually by uploading the test package once more.