Code reorg, v1#234
Conversation
This moves all directory information into a new Config class that captures zkg's internal configuration, based on its config file. The motivation here is that the Manager class does not need to manage this information, and by making it available from a central configuration, other classes can also leverage it (as opposed to having to live in the Manager). It includes various consistency fixes: the zeek_dist path is only used when actually present, saving the configuration always uses a canonical ordering (alphabetically, first by sections, then by options in each section), and some naming is standardized (all path naming was in plurals but "clones/package" and "clones/source", which are now "clones/packages" and "clones/sources", respectively). This breaks backward compatibility in the code, but the naming was inconsistent, and zeekpkg hasn't been intended as an module API to date.
The tests shouldn't reach out to Github for the default package source, unless they explicitly need to.
Given that we already have a class for package sources, it makes more sense for that functionality to live in that class, instead of the manager. This also introduces a small tweak to the behavior of the refresh command, prohibiting the use of "--push" without "--aggregate". There is no other local modification to source repo clones.
Also, no longer create the zkg.test_command logs buried in the test clone. This mirrors how we already produce the build log, for packages that have a build_command.
This avoids circular dependencies when removing the amount of from-import statements into the toplevel zeekpkg module, in the next changeset.
The zeekpkg.cli module produces the argument parser and contains the various commands. It leaves a single print() in zkg.py (redundantly to _util.print_error()), which is temporary until we introduce a UI abstraction.
This further shrinks the toplevel zkg script.
The "zkg list" command would previously preserve any opening quotation marks, but since it cuts off the description after the first sentence this would look broken. Such surrounding quotes are now never rendered in the output.
This provides UserInterface, an abstract base class for user input/output. Implementations can adapt to the environment (TTY, file I/O, etc). It includes one implementation, PlainUI, for basic plaintext I/O. This also generalizes the notion of InstallWorker into generic activities undertaken by zkg, which allows treating trackable ones via a progress bar, etc. This does not alter the current split of messages to stdout/stderr. It tweaks the message prefix slightly for consistency, which touches a few baselines but makes no deeper changes otherwise. This also adds a UI proxy abstraction to allow runtime adjustment of UI implementation. Reassigning the toplevel global would not apply to the copy of the UI object imported into other modules.
b2b7a57 to
61783ff
Compare
| These are zkg's package inventories. Each source gets a subdirectory | ||
| associated with its name. The default source is called "zeek". | ||
| """ | ||
| return os.path.join(self.state_dir(), "clones", "sources") |
There was a problem hiding this comment.
This was called clones/source previously and since we don't have migration logic in place this will silently break existing zkg installations. I understand that the renaming makes things more tidy, but I think we should either avoid breaking users for such a minor improvement and skip this rename, or add migration logic. The first would probably be simpler.
|
|
||
| Each package gets a subdirectory associated with its name. | ||
| """ | ||
| return os.path.join(self.state_dir(), "clones", "packages") |
| LOG.setLevel(logging.INFO) | ||
| elif verbosity == 2: | ||
| LOG.setLevel(logging.WARNING) | ||
| elif verbosity >= 3: |
There was a problem hiding this comment.
This is weird, I'd expect WARNING to be less verbose than INFO. Did you accidentally swap these or did I miss something?
| } | ||
|
|
||
| substitutions.update(self.user_vars) | ||
| if CONFIG.zeek_dist() is not None: |
There was a problem hiding this comment.
Config.zeek_dist returns a str and implements a fallback so it currently can never return None and this seems unreachable; there's also a XXX TODO in the code there. Suggest to remove the fallback and clearly communicate the unset value up by returning str | None. Only issue is that our fallback here then uses Config.zeek_dist which also returns a str and has a similar fallback, so we might need to deal with unset values somewhere else completely anyway.
| # def items_without_defaults(section: str) -> list[tuple[str, str]]: | ||
| # # Same as ConfigParser.items(section), but without default keys. | ||
| # defaults = {key for key, _ in self.items("DEFAULT")} | ||
| # items = sorted(self.items(section)) | ||
| # return [(key, val) for (key, val) in items if key not in defaults] | ||
|
|
| # XXX instead of magic handling of tarfiles, it'd be nicer | ||
| # to have separate keywords for shipping archives. | ||
| # Since it only seems to make sense for pre-built plugins | ||
| # and their dependencies, perhaps plugin_archive. |
There was a problem hiding this comment.
I suspect this could be implemented on top of #236, maybe let's add a comment there and I can work on incorporating it, or let's add an issue sketching what you had in mind.
| verbosity (int): the log level. Values 0-3 increase logging, larger | ||
| values make no difference. | ||
|
|
||
| rich_logging (bool): whether to use timestamped, log-style log formatting. |
There was a problem hiding this comment.
We don't have this parameter, is this stale or did you want to add it?
| ) | ||
|
|
||
|
|
||
| class Config(configparser.ConfigParser): |
There was a problem hiding this comment.
Could you add a class docstring and also ones for the interesting methods? Right now this is pretty bare-bones.
There was a problem hiding this comment.
Let's add a module-level docstring here.
| sys.exit(1) | ||
|
|
||
| # Dictionary for storing package info to output as JSON | ||
| pkginfo: dict[str, Any] = {} |
There was a problem hiding this comment.
Just a note, nothing to do: You just moved this of Any around (same for the use in _fill_metadata_version below), but we should remember to use proper types for new code instead of sidestepping typechecking with Any.
One way to do that would be to introduce proper types like e.g.,
PkgMetadataVersion = dict[str, str | dict[str, str]]
class PkgInfoEntry(TypedDict, total=False):
metadata: dict[str, PkgMetadataVersion]
invalid: str
url: str
versions: list[str]
install_status: dict[str, str]
metadata_file: strWith that we'd have a pkginfo: dict[str, PkgInfoEntry] = {} here, and pass PkgMetadataVersion to _fill_metadata_version instead of Any. Our currently handling is inconsistent, so we'd need to e.g., add some assertions to make clear what exact shape the data we work with has. But in any case, no need to worry about that in this branch.
2 participants
@bbannier this is the bulk of the changes I've been sitting on that we've discussed. In essence it's moving all of the commands out of zkg into zeekpkg.cli, the introduction of a config class, a UI abstraction, and some minor tweaks.
Some minor behavior changes slipped in there that don't need to be there, such as 04a8580. We could postpone these until later — I don't feel strongly.
I'll make this a draft PR since I imagine you have opinions about it, and I don't mean for this to cause conflicts with other PRs in flight.