-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fixes #55
Conversation
@soniamitchell - can you try running this to make sure you can still import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! 👍
I need this merged with dev to test run --ci |
fair/session.py
Outdated
# Populate file type table | ||
_local_uri = fdp_conf.get_local_uri() | ||
_local_uri = self._global_config['registries']['local']['uri'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask why have you changed this line? The function get_local_uri
does this anyway, but also catches the exception to print it in the same style as all errors in the CLI (hides the traceback unless --debug
is used) and ensures it can be called from anywhere. Using a global function is better than hardcoding as it means should the layout of things change, only one place has to be altered.
def get_local_uri() -> str:
_cfg = read_global_fdpconfig()
try:
return _cfg['registries']['local']['uri']
except KeyError:
raise fdp_exc.CLIConfigurationError(
f"Expected key 'registries:local:uri' in local CLI configuration"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't do the same actually - I suspect because get_local_uri()
relies on read_global_fdpconfig()
which relies on the file being written rather than being in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kzscisoft - I totally agree this is inelegant btw, happy to have a fix, but I just don't think it can involve get_local_uri()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't do the same actually - I suspect because
get_local_uri()
relies onread_global_fdpconfig()
which relies on the file being written rather than being in memory?
Hmm but is written a couple of lines above:
with open(fdp_com.global_fdpconfig(), "w") as f:
yaml.dump(self._global_config, f)
Is this write failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may seem a bit weird to re-read the same thing, but it is a failsafe in a way in that it tests the write actually did happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah. Well, that's weird. It works now. I suspect that the file wasn't getting written out correctly because it was looking for a description
key somewhere, and I got rid of that requirement - when I looked the file existed but was empty, so I assumed that it was part way through writing for some reason. Anyway, I'll happily undo this change then.
Kudos, SonarCloud Quality Gate passed!
|
Bugfixes:
--glob
with--global
forfair purge
, closesfair purge
takes a --glob argument when it should be --global #35description
key incli-config.yaml
files sofair init --export <file.yaml>
doesn't work, closesfair init --export <file.yaml>
no longer works #54fair init --using <file.yaml>
, closesfair init --using <file.yaml>
no longer works #56