Open turbomam opened 7 months ago
This target doesn't create an output file in the CWD called
migrator
does it?
No, it creates a file called migrator_from_...
and it creates it in the migrators/
folder.
Could the migration bootstrapping script take a name or output path as an argument?
Generating a file name that is consistent with the schema version numbers involved is one of the services the script provides: given a pair of version numbers, it generates a valid file name into which those version numbers are incorporated. From a technical standpoint, the script could be updated to use a user-specified file name and/or path, but it currently does not do so.
If it doesn't, then we should add
OK, I'll open a PR with that change. Thanks for pointing it out to me.
It is listed as a .PHONY:
dependency.
It is a few lines up (above the other migrator-related target).
.PHONY: migration-doctests migrator
Do you want me to move it to a separate .PHONY:
line that is directly above the migrator
target, or is the current location sufficient? Either one works for me.
Thanks for checking. No, we don't have to move the .PHONY
assertion.
OK, I like the feedback from click.echo(f"Created '{file_name}' successfully.")
, but I'd prefer that collection of the from
and to
versions would also be available as command line arguments, so that they can be illustrated in the makefile target.
I'd prefer that collection of the from and to versions would also be available as command line arguments, so that they can be illustrated in the makefile target.
They are also command-line options. Because they are defined with the prompt
kwarg set (e.g. prompt="Some prompt"
), if the user doesn't include those command-line options, the script will prompt the user for them. This way, people don't have to edit the project.Makefile
every time they want to generate a new migrator (alternatively, they could run $ poetry run create-migrator
directly).
You can see all the command-line options by either looking at the script's code or running the script with the --help
command-line option.
root@82a58c1ab0c4:/nmdc-schema# poetry run create-migrator --help
Usage: create-migrator [OPTIONS]
Create a migrator class based upon the specified version identifiers.
Options:
--from-version, --from TEXT Schema version from which the migrator will
migrate data (e.g. "1.0"). [required]
--to-version, --to TEXT Schema version to which the migrator will
migrate data (e.g. "1.1"). [required]
--help Show this message and exit.
Here's a link to the documentation about the prompt
kwarg of click
: https://click.palletsprojects.com/en/8.1.x/options/#prompting
We try to avoid makefile targets that aren't explicit about the files they create.
I'm comfortable with removing the script from the Makefile altogether, in case you don't think it fits in well with the existing patterns. People will still be able to run the script via $ poetry run create-migrator
, and I could update the migration documentation to show that command instead of the make
command.
I'm comfortable with removing the script from the Makefile altogether,
Is it mentioned in any of the maintainer/contributor documents in the root of the repo?
Is it mentioned in any of the maintainer/contributor documents in the root of the repo?
No, it (the make migrator
command) is not mentioned in any of: README.md
, CONTRIBUTING.md
, DEVELOPMENT.md
, MAINTAINERS.md
.
It is only mentioned here (confirmed via GitHub search): https://github.com/microbiomedata/nmdc-schema/blob/58e93e69daaae71ed4c478192082f604696467b9/nmdc_schema/migrators/README.md?plain=1#L29
I don't know what remains for this issue. Hi @turbomam, based upon our exchange above, are you OK with closing this issue—or is there action that you want me to take regarding it?
Closing as I think there are no outstanding tasks here.
Ideally the makefile target would illustrate the use of command line arguments, not just launch it in default mode.
Given how the script works today, I see a few options:
make migrator
command that is interactiveMy first choice is 3, then 2, then (big gap, since 1 requires recurring maintenance) 1.
What do you think?
that documentation already exists
This target doesn't create an output file in the CWD called
migrator
does it?If it doesn't, then we should add
on the line above it or below it
Could the migration bootstrapping script take a name or output path as an argument?
We try to avoid makefile targets that aren't explicit about the files they create.