fix: add version to samconfig.toml file
#1581
Conversation
samcli/lib/config/samconfig.py
Outdated
|
|
||
| def _version_sanity_check(self, version): | ||
| if not isinstance(version, float): | ||
| raise SamConfigVersionException(f"'{VERSION_KEY}' key is not present " f"or is in unrecognized format. ") |
There was a problem hiding this comment.
This is well and good, but what is the purpose of a version number? The AWS CLI config has never needed this, what does it mean when we increment it?
There was a problem hiding this comment.
This was discussed as part of design, but at the very least, it provides us a mechanism depending on the growth of scope of the configuration file in the future.
|
|
||
| except SamConfigVersionException as ex: | ||
| LOG.debug("%s %s", samconfig.path(), str(ex)) | ||
| raise ConfigException(f"Syntax invalid in samconfig.toml: {str(ex)}") |
There was a problem hiding this comment.
Why change Exception type? What is difference between SamConfigVersionException and ConfigException?
Also probably include in exception message that this is due to Invalid Version instead of Invalid Syntax.
There was a problem hiding this comment.
One is a click exception, and the other isnt. str(ex) gives us version based information.
| self.document = tomlkit.loads(txt) | ||
| self._version_sanity_check(self._version()) | ||
| except OSError: | ||
| self.document = tomlkit.document() |
There was a problem hiding this comment.
nitpick: Probably add a debug log before default to tomlkit.document().
There was a problem hiding this comment.
Sure. Edit: actually lets think about that a bit more, if there is no config file we say we couldn't find it every-time under debug, I'm not sure if thats the behavior we want.
| return self.document | ||
|
|
||
| def _write(self): | ||
| if not self.document: | ||
| return | ||
| if not self.exists(): | ||
| open(self.filepath, "a+").close() | ||
| current_version = self._version() if self._version() else SAM_CONFIG_VERSION | ||
| try: |
There was a problem hiding this comment.
Just curious: any reason why we are not checking for key to be present and add only if it isn't?
There was a problem hiding this comment.
| current_version = self._version() if self._version() else SAM_CONFIG_VERSION | ||
| try: | ||
| self.document.add(VERSION_KEY, current_version) | ||
| except tomlkit.exceptions.KeyAlreadyPresent: |
There was a problem hiding this comment.
We check it here.
- support version key, any float is okay. - if a config file is present and the version key is missing, we do not process it. - if a config file is missing, thats fine. this check does not get in the way. - validation logic to determine if a SAM CLI version is compatible can be written later.
sriram-mv
force-pushed
the
toml_version
branch
from
f1d0586 to
a0d573a
Compare
| if not self.document: | ||
| self._read() |
There was a problem hiding this comment.
bugfix: do not attempt to read everytime a put is made, read once and load.
* fix: add version to `samconfig.toml` file - support version key, any float is okay. - if a config file is present and the version key is missing, we do not process it. - if a config file is missing, thats fine. this check does not get in the way. - validation logic to determine if a SAM CLI version is compatible can be written later. * bugfix: do not continously read everytime a samconfig.put is called
process it.
the way.
be written later.
Issue #, if available:
Description of changes:
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.