feat: import resources from git repo or archive file

[keithmanville]

resolves:

• Add a workflow for importing a plugin from a git repository or file upload #588
• Create a simple "hello-world" plugin #591
• Cleanup dioptra built-ins #739
• Cleanup deprecated examples #740

[keithmanville]

Basic import functionality is now working. To test it with the included hello world plugin, go to the /api/v1/workflows/resourceImport endpoint in swagger, then either import via git or upload:

for git, set sourceType to "git" and gitUrl to https://github.com/usnistgov/dioptra#workflow-import

for file upload, first create an archive containing the necessary files (from the root dioptra dir in this branch)

tar -czf plugins.tar.gz -C . plugins examples/hello-world.yaml dioptra.toml

then set sourceType to "upload" and select the archive you created.

[keithmanville]

converting back to Draft.

Needs to update the client to support the new workflow. Tests should be updated to use client.

Blocked by #694

[jkglasbrenner]

The core behavior and functionality worked for me when I tested things, so that's good I am able to reproduce things on my machine. Besides some refactoring suggestions, which should be straightforward, I noticed a few places that we weren't validating user-provided input, such as whether the user-provided filepaths for an import are legal. I left commentary to point out what we should do, and if there was any examples already in the code base that you can refer to.

[jkglasbrenner]

While it works if you include the parentheses, my IDE's static type checker does not like including them. This should also work:

Suggested change

	@pyplugs.register()
	@pyplugs.register

[keithmanville]

fixed here: 3bd3e2a

[jkglasbrenner]

Wanted to verify if there was a preference to giving this the longer name dioptra-resources.toml instead of the simpler dioptra.toml that is implied by the default value in the API.

[keithmanville]

No strong preference either way for me, but I should keep it consistent.

Should I just stick with dioptra.toml?

[keithmanville]

I just made it dioptra.toml: 7f51ce1

[jkglasbrenner]

Somewhere we need to do a check that the git command exists, and raise an informative error message if it doesn't. The shutil.which command is one way to do this (it also tries to find the absolute path to the command, which I think is safer to pass to a subprocess call), here's an example from tests/unit/restapi/lib/server.py:

dioptra/tests/unit/restapi/lib/server.py

Lines 112 to 118 in 9e56076

	cmd = shutil.which("flask")
	if cmd is None:
	raise RuntimeError("Flask command not found.")
	return cmd

[keithmanville]

added this here: f258bc8
also added a docstring for the clone_fit_repository function here: 90f4139

[jkglasbrenner]

While giving feedback on #719, I revisited the use and purpose of the @overload decorator, and my understanding is now that its primarily used to disambiguate function signatures like the following:

def myfunc(a: str | int) -> str | float:
   ...

when, for example, you know that the only outcomes are str returns str or int returns float.

The import_resources method always returns DioptraResponseProtocol, so there's nothing to disambiguate. My IDE's static type checker is yelling that it doesn't agree with the use of @overload here either.

Basically, I don't think we can use @overload to signify that the git_url, archive_file, and files arguments are mutually exclusive. I think I was the one that suggested using @overload like this in the first place, so that's on me.

Looking at this again, the type for git_url, archive_file, and files is unique, indicating that the type itself can be used to determine what you're uploading. What if we just do something like this for the function signature:

def import_resources(
    self,
    group_id: int,
    source: str | DioptraFile | list[DioptraFile],
    config_path: str = "dioptra.toml",
    resolve_name_conflicts_strategy: Literal["fail", "overwrite"] = "fail",
) -> DioptraResponseProtocol:

And then lines 189-199 could be updated as follows:

if not isinstance(source, (str, DioptraFile, list)):
    raise SomeError

if isinstance(source, list):
    if not all([isinstance(x, DioptraFile) for x in source]):
        raise SomeError

    data["sourceType"] = "upload_files"
    files_["files"] = source

if isinstance(source, str):
    data["sourceType"] = "git"
    data["gitUrl"] = source

if isinstance(source, DioptraFile):
    data["sourceType"] = "upload_archive"
    files_["archiveFile"] = source

What do you think?

[keithmanville]

took your suggestion: 614f5db

[jkglasbrenner]

Are these arguments optional in the API itself and fill in their own defaults? If so, then maybe the default values for config_path and resolve_name_conflicts_strategy should both be None.

[keithmanville]

They do have defaults in the API, so they could default to None here and have the same behavior. I replicated the defaults in the client, so it is clear to the user what the default behavior is without needing to look up the API docs. However, this may be inconsistent with other parts of the client.

[jkglasbrenner]

Is this hash independent of the order you pass the list of files? I don't think it is.

[keithmanville]

You're right. Attempted to resolve this by sorting by file path: 844cb1c

I'm not sure if there is a better strategy.

[jkglasbrenner]

If we must change the directory, we should control it via a context manager. I don't see this reversed anywhere, so its long-term effect on the REST API's processes/threads could be unpredictable.

The SDK has the following contextmanager, although if we don't want to import from that module within the restapi, we can duplicate it into the restapi utils.py:

dioptra/src/dioptra/sdk/utilities/paths/set_cwd.py

Lines 23 to 39 in 3b81da4

	@contextlib.contextmanager
	def set_cwd(temp_cwd: Union[str, Path]) -> Iterator[None]:
	"""
	A convenient way to temporarily set the current working directory to a
	particular directory, while executing a particular block of code. Just
	place the code into a with-statement which uses this function as a
	contextmanager.
	Args:
	temp_cwd: The directory to temporarily set as current
	"""
	saved_cwd = Path.cwd()
	os.chdir(temp_cwd)
	try:
	yield
	finally:
	os.chdir(saved_cwd)

[keithmanville]

f5131ef

[jkglasbrenner]

Like mentioned elsewhere, I'd use a contextmanager to control the lifetime of a changed directory:

dioptra/src/dioptra/sdk/utilities/paths/set_cwd.py

Lines 23 to 39 in 3b81da4

	@contextlib.contextmanager
	def set_cwd(temp_cwd: Union[str, Path]) -> Iterator[None]:
	"""
	A convenient way to temporarily set the current working directory to a
	particular directory, while executing a particular block of code. Just
	place the code into a with-statement which uses this function as a
	contextmanager.
	Args:
	temp_cwd: The directory to temporarily set as current
	"""
	saved_cwd = Path.cwd()
	os.chdir(temp_cwd)
	try:
	yield
	finally:
	os.chdir(saved_cwd)

[keithmanville]

f5131ef

[jkglasbrenner]

Like mentioned elsewhere, I'd use a contextmanager to control the lifetime of a changed directory:

dioptra/src/dioptra/sdk/utilities/paths/set_cwd.py

Lines 23 to 39 in 3b81da4

	@contextlib.contextmanager
	def set_cwd(temp_cwd: Union[str, Path]) -> Iterator[None]:
	"""
	A convenient way to temporarily set the current working directory to a
	particular directory, while executing a particular block of code. Just
	place the code into a with-statement which uses this function as a
	contextmanager.
	Args:
	temp_cwd: The directory to temporarily set as current
	"""
	saved_cwd = Path.cwd()
	os.chdir(temp_cwd)
	try:
	yield
	finally:
	os.chdir(saved_cwd)

[keithmanville]

f5131ef

[jkglasbrenner]

Use something like this for status codes:

from http import HTTPStatus

assert response.status_code == HTTPStatus.OK

[keithmanville]

Good catch. I think this code may have actually been written before Harold refactored the errors module and took Andy's suggestion of using the HTTPStatus codes.

9762690