add hotfix for gitlab extensions #257
Conversation
| @@ -517,7 +520,7 @@ def from_dict( | |||
| return cls( | |||
| name=name, | |||
| script=script, | |||
| image=node.get("image"), | |||
| image=_get_str(node, "image"), | |||
There was a problem hiding this comment.
This fix is actually independent from the !reference constructor addition, but only the latter is mentioned anywhere.
At a minimum, please update the commit message and/or PR description to call this out explicitly. The word "image" doesn't appear anywhere but the diff.
Bonus points if you put this (and the associated test addition) into their own commit. More bonus points if it is its own PR -- since this isn't really a GitLab "extension" to yaml, it's just a part of their schema that Scuba doesn't (yet) support.
|
|
||
| Loader.add_constructor("!from_yaml", Loader.from_yaml) | ||
| Loader.add_constructor("!override", Loader.override) | ||
| Loader.add_constructor("!reference", Loader.not_yet_implemented) |
There was a problem hiding this comment.
Can you update your PR description to reference #200 which spells out in great detail what the !reference tag is, and how GitLab uses it?
| @@ -155,9 +155,12 @@ def override(self, node: yaml.nodes.Node) -> OverrideMixin: | |||
| assert isinstance(obj, OverrideMixin) | |||
| return obj | |||
|
|
|||
| def not_yet_implemented(self, node: yaml.nodes.Node): | |||
There was a problem hiding this comment.
Let's name this !reference, following the existing pattern. That it is not yet implemented is mostly inconsequential.
| @@ -155,9 +155,12 @@ def override(self, node: yaml.nodes.Node) -> OverrideMixin: | |||
| assert isinstance(obj, OverrideMixin) | |||
| return obj | |||
|
|
|||
| def not_yet_implemented(self, node: yaml.nodes.Node): | |||
| pass | |||
There was a problem hiding this comment.
This implementation appears to just ignore the !reference tags, without raising an error or warning the user, simply returning None (implicitly). This seems like a huge problem to me.
Consider this example:
.setup:
script:
- do-something-really-important
build:
stage: build
script:
- !reference [.setup, script]
- depends-on-important-stuffThe result (AFICT) is a script of [None, "depends-on-important-stuff"].
I suspect that will crash somewhere later because of the None value.
If we pretend for a second that the None value is ignored, then the resulting script will be missing the first step, and executing anyway, despite the YAML author's intentions.
I don't see how this can be better than the current state of affairs (in which it just crashes).
|
|
||
| Loader.add_constructor("!from_yaml", Loader.from_yaml) | ||
| Loader.add_constructor("!override", Loader.override) | ||
| Loader.add_constructor("!reference", Loader.not_yet_implemented) |
There was a problem hiding this comment.
!reference is only added for compatibility with content imported from .gitlab-ci.yml (via !from_yaml), correct? It is not a part of the documented .scuba.yml schema.
So I don't think we should be adding this constructor to this Loader class. It should be part of a separate Loader class used only when parsing .gitlab-ci.yml files. And how do we know we're parsing a .gitlab-ci.yml file (or a differently-named file using its schema)? We should use a new tag + constructor which is a variant of !from_yaml.
This is all spelled out in #200.
| """ | ||
| ) | ||
|
|
||
| def test_load_config_gitlab_extensions(self) -> None: |
There was a problem hiding this comment.
What are GitLab "extensions"? AFAICT this isn't a term that's used anywhere in YAML, Scuba, or GitLab documentation. If this is testing the !reference tag, this should probably have "reference" in the name.
| """ | ||
| ) | ||
|
|
||
| def test_load_config_gitlab_extensions(self) -> None: |
There was a problem hiding this comment.
Many of the other tests have a docstring that briefly describes what is being verified. Please consider adding that here as well.
| two: | ||
| stage: build | ||
| image: | ||
| name: dummian:8.2 |
There was a problem hiding this comment.
It is not obvious what exactly is being tested here. I had to read it several times to figure it out.
A comment like this would go a long way in aiding understanding.
# .gitlab-ci.yml supports a long-form `image` specification
# with `name` and other sub-keys:
# https://docs.gitlab.com/ee/ci/yaml/#image
#
# Scuba doesn't currently support that, so make sure we handle
# this error gracefully.
#
# TODO(#230): Support this complex `image` format.| """ | ||
| ) | ||
|
|
||
| load_config(config_text=f"image: !from_yaml {GITLAB_YML} normal.image") |
There was a problem hiding this comment.
Most other tests go on to validate the contents of the object returned from load_config. If you do that, I think you'll find that the results are unsatisfactory.
|
I think I was trying to do to much with this one branch (not a single-issue branch). I'm going to close this in favor of a PR that fixes #229 only, and take the comments you've left as guidance for the more in-depth/proper handling of a gitlab yaml loader |
Fix crashing issue (#229)
Adds a
not_yet_implentedfunc to 'handle'!referencetags from gitlabI'm currently working on a proper implementation for both of these issue, but this should work as a stop-gap until then.