| |
| ▲ | spott 5 hours ago | parent | next [-] | | The counterpoint is that I don’t want to be an expert in GitHub actions to not repeat myself. YAML anchors are a standard that I can learn and use in a lot of places. The idiosyncrasies of GitHub actions aren’t really useful for me to learn. Just my $0.02 | |
| ▲ | detaro 8 hours ago | parent | prev | next [-] | | in what ways do they make static analysis harder? Don't they flatten out trivially after parsing? | | |
| ▲ | woodruffw 8 hours ago | parent [-] | | The flattening out is the problem: most (all?) widely used YAML parsers represent the YAML document using the JSON object model, which means that there's no model or element representation of the anchors themselves. That in turn means that there's no way to construct a source span back to the anchor itself, because the parsed representation doesn't know where the anchor came from (only that it was flattened). | | |
| ▲ | VGHN7XDuOXPAzol 8 hours ago | parent [-] | | This is something that a custom parser library could figure out, no? The same as how you have format-preserving TOML libraries, for instance. I think it makes way more sense for GitHub to support YAML anchors given they are after all part of the YAML spec. Otherwise, don't call it YAML! (This was a criticism of mine for many years, I'm very glad they finally saw the light and rectified this bug) | | |
| ▲ | woodruffw 8 hours ago | parent [-] | | > This is something that a custom parser library could figure out, no? The same as how you have format-preserving TOML libraries, for instance. Yes, it's just difficult. The point made in the post isn't that it's impossible, but that it significantly changes the amount of of "ground work" that static analysis tools have to do to produce useful results for GitHub Actions. > I think it makes way more sense for GitHub to support YAML anchors given they are after all part of the YAML spec. Otherwise, don't call it YAML! (This was a criticism of mine for many years, I'm very glad they finally saw the light and rectified this bug) It's worth noting that GitHub doesn't support other parts of the YAML spec: they intentionally use their own bespoke YAML parser, and they don't have the "Norway" problem because they intentionally don't apply the boolean value rules from YAML. All in all, I think conformance with YAML is a red herring here: GitHub Actions is already its own thing, and that thing should be easy to analyze. Adding anchors makes it harder to analyze. | | |
| ▲ | VGHN7XDuOXPAzol 7 hours ago | parent [-] | | > conformance with YAML maybe, but not entirely sure. 'Two wrongs don't make a right' kind of thinking on my side here. But if they call it GFY and do what they want, then that would probably be better for everyone involved. > they don't have the "Norway" problem because they intentionally don't apply the boolean value rules from YAML. I think this is YAML 1.2. I have not done or seen a breakdown to see if GitHub is aiming for YAML 1.2 or not but they appear to think that way, given the discussion around merge keys -- (though it's still not clear why flattening the YAML would not be sufficient for a static analysis tool. If the error report references a key that was actually merged out, I think users would still understand the report; it's not clear to me that's a bad thing actually) | | |
| ▲ | woodruffw 7 hours ago | parent [-] | | > But if they call it GFY and do what they want, then that would probably be better for everyone involved. Yes, agreed. > I think this is YAML 1.2. I have not done or seen a breakdown to see if GitHub is aiming for YAML 1.2 or not but they appear to think that way, given the discussion around merge keys I think GitHub has been pretty ambiguous about this: it's not clear to me at all that they intend to support either version of the spec explicitly. Part of the larger problem here is that programming language ecosystems as a whole don't consistently support either 1.1 or 1.2, so GitHub is (I expect) attempting to strike a happy balance between their own engineering goals and what common language implementations of YAML actually parse (and how they parse it). None of this makes for a great conformance story :-) > (though it's still not clear why flattening the YAML would not be sufficient for a static analysis tool. If the error report references a key that was actually merged out, I think users would still understand the report; it's not clear to me that's a bad thing actually) The error report includes source spans, so the tool needs to map back to the original location of the anchor rather than its unrolled document position. (This is table stakes for integration with formats like SARIF, which expect static analysis results to have physical source locations. It's not good enough to just say "there's a bug in this element and you need to find out where that's introduced," unfortunately.) |
|
|
|
|
| |
| ▲ | willseth 8 hours ago | parent | prev | next [-] | | I think the main reason you see overwhelming support for anchors is that the existing Actions functionality is typically so cumbersome to implement and often makes it harder to understand a workflow. Anchor syntax is a little esoteric, but otherwise very simple and grokable. | | |
| ▲ | woodruffw 8 hours ago | parent [-] | | To be clear, I understand why people want to use anchors. The argument isn't that they aren't useful: it's that the juice is not worth the squeeze, and that GitHub's decision to support them reflects a lack of design discretion. Or in other words: if your problem is DRYness, GitHub should be fixing or enhancing the ~dozen other ways in which the components of a workflow shadow and scope with each other. Adding a new cross-cutting form of interaction between components makes the overall experience of using GitHub Actions less consistent (and less secure, per points about static analysis challenges) at the benefit of a small amount of deduplication. | | |
| ▲ | verdverm 4 hours ago | parent | next [-] | | > GitHub's decision to ... reflects a lack of design discretion. So true, not the first time, not the last time. I'm at the point of exploring Gerrit as an alternative | |
| ▲ | btreecat 8 hours ago | parent | prev [-] | | So GitHub shouldn't implement the spec because you personally don't like that the spec solves a problem you can optionally solve at another layer? | | |
| ▲ | woodruffw 8 hours ago | parent | next [-] | | No; GitHub shouldn't support YAML anchors because it's a deviation from the status quo, and the argument is specifically that the actions ecosystem doesn't need to make analysis any harder than it already is. (As the post notes, neither I nor GitHub appears to see full compliance with YAML 1.1 to be an important goal: they still don't support merge keys, and I'm sure they don't support all kinds of minutiae like non-primitive keys that make YAML uniquely annoying to analyze. Conforming to a complex specification is not inherently a good thing; sometimes good engineering taste dictates that only a subset should be implemented.) | | |
| ▲ | btreecat 8 hours ago | parent | next [-] | | > No; GitHub shouldn't support YAML anchors because it's a deviation from the status quo, and the argument is specifically that the actions ecosystem doesn't need to make analysis any harder than it already is.
>
> (As the post notes, neither I nor GitHub appears to see full compliance with YAML 1.1 to be an important goal: they still don't support merge keys, and I'm sure they don't support all kinds of minutiae like non-primitive keys that make YAML uniquely annoying to analyze. Conforming to a complex specification is not inherently a good thing; sometimes good engineering taste dictates that only a subset should be implemented.) That's a long way to say "yes, actually" | | |
| ▲ | woodruffw 8 hours ago | parent [-] | | > That's a long way to say "yes, actually" "Because I don't like it" makes it sound like I don't have a technical argument here, which I do. Do you think it's polite or charitable to reduce peoples' technical arguments into "yuck or yum" statements like this? |
| |
| ▲ | VGHN7XDuOXPAzol 8 hours ago | parent | prev [-] | | > Conforming to a complex specification is not inherently a good thing Kind of a hard disagree here; if you don't want to conform to a specification, don't claim that you're accepting documents from that specification. Call it github-flavored YAML (GFY) or something and accept a different file extension. https://github.com/actions/runner/issues/1182 > YAML 1.1 to be an important goal: they still don't support merge keys right, they don't do merge keys because it's not in YAML 1.2 anymore. Anchors are, however. They haven't said that noncompliance with YAML 1.2 spec is intentional | | |
| ▲ | woodruffw 7 hours ago | parent [-] | | > Call it github-flavored YAML (GFY) or something and accept a different file extension. Sure, I wouldn't be upset if they did this. To be clear: there aren't many fully conforming YAML 1.1 and 1.2 parsers out there: virtually all YAML parsers accept some subset of one or the other (sometimes a subset of both), and virtually all of them emit the JSON object model instead of the internal YAML one. |
|
| |
| ▲ | 8 hours ago | parent | prev [-] | | [deleted] |
|
|
| |
| ▲ | GuinansEyebrows 8 hours ago | parent | prev [-] | | is your criticism leveled at yaml anchors or github? in my anecdotal experience, yaml anchors were a huge help (and really, really not hard to grasp at a conceptual level) in maintaining uniform build processes across environments. | | |
| ▲ | woodruffw 8 hours ago | parent [-] | | It is specifically leveled at YAML anchors in GitHub. I don't have a super strong opinion of YAML anchors in other contexts. (This post is written from my perspective as a static analysis tool author. It's my opinion from that perspective that the benefits of anchors are not worth their costs in the specific context of GitHub Actions, for the reasons mentioned in the post.) | | |
| ▲ | saurik 7 hours ago | parent [-] | | "YAML" should mean something. When I saw GitHub Actions supported "YAML", I thought "OK, certainly not my favorite, but I can deal with that", and so I read the YAML specification, saw anchors, and then had to realize the hard way that they didn't work on GitHub Actions, leaving me unsure what even would or wouldn't work going forward. Is this even the only way it differs? I don't know, as they apparently don't use YAML :/. This also means that, if you use an off-the-shelf implementation to parse these files, you're "doing it wrong", as you are introducing a parser differential: I can put code in one of these files that one tool uses and another tool ignores. (Hopefully, the file just gets entirely rejected if I use the feature, but I do not remember what the experience I had was when I tried using the feature myself; but, even that is a security issue.) > Except: GitHub Actions doesn’t support merge keys! They appear to be using their own internal YAML parser that already had some degree of support for anchors and references, but not for merge keys. Well, hopefully they also prioritize fixing that? Doing what GitHub did, is apparently still doing, and what you are wanting them to keep doing (just only in your specific way) is not actually using "YAML": it is making a new bespoke syntax that looks a bit like YAML and then insisting on calling it "YAML" even though it isn't actually YAML and you can neither read the YAML documentation nor use off-the-shelf YAML libraries. Regardless, it sounds like your tool already supports YAML anchors, as your off-the-shelf implementation of YAML (correctly) supports YAML anchors. You are upset that this implementation doesn't provide you source map attribution: that was also a problem with C preprocessors for a long time, but that can and should be fixed inside of the parser, not by deciding the language feature shouldn't exist because of library limitations. | | |
| ▲ | kiitos 7 hours ago | parent | next [-] | | > and so I read the YAML specification but there isn't a single YAML spec, there are at least 2 in common use: yaml 1.1, and 1.2, which have discrete specs and feature-sets. re: anchor stuff specifically, 1.1 supports merge keys whereas 1.2 explicitly does not, so that's one thing and github actions does not actually specify which yaml spec/version it uses when parsing workflow yaml files it's unfortunately just not the case that "YAML means something" that is well-defined in the sense that you mean here | |
| ▲ | woodruffw 7 hours ago | parent | prev [-] | | > "YAML" should mean something. Sure, agreed. Another comment notes that GitHub probably should call this their own proprietary subset of YAML, and I wouldn't object to that. > Well, hopefully they also prioritize fixing that? I expect they won't, since it's not clear what version of YAML they even aim to be compatible with. However, I don't understand why engineers who wouldn't jump off of a bridge because someone told them to would follow a spec to the dot just because it exists. Specifications are sometimes complicated and bad, and implementing a subset is sometimes the right thing to do! GitHub Actions, for example, doesn't make use of the fact that YAML is actually a multi-document format, and most YAML libraries don't gracefully handle multiple documents in a single YAML stream. Should GitHub Actions support this? It's entirely unclear to me that there would be any value in them doing so; subsets are frequently the right engineering choice. |
|
|
|
|
