Skip to content
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

Consider reverting ".recipe.yaml" file extension requirement for YAML-formatted recipes #767

Open
homebysix opened this issue Sep 8, 2021 · 31 comments

Comments

@homebysix
Copy link
Member

Summary

Proposal for consideration: revert the requirement introduced in AutoPkg 1.3.0 that YAML recipes must end with ".recipe.yaml" and adding support for ".recipe.plist". Instead, return to ".recipe" as the required extension for AutoPkg recipes, as was the case from the inception of AutoPkg up to version 1.3.0.

NOTE: This issue does not encompass actual YAML recipe support, only the file extension support that came with it.

Context

When YAML support was added to AutoPkg (#617 and #698), a design decision about recipe extensions was made:

Recipe filename extensions
With this change comes a design decision about recipe filename extensions. I've chosen to infer recipe format using a .recipe.yaml extension instead of trying to parse files with the .recipe extension multiple ways. This requires recipe authors to ensure that yaml recipe filenames end with .recipe.yaml. Recipes that end in .recipe will continue to be treated like plists. (A new .recipe.plist extension is also allowable.)

In addition to the reason provided above, the .yaml extension allows text editors to parse and display AutoPkg recipes automatically, which is an important feature for AutoPkg recipe authors since it allows error-catching and autofill capability.

Using literal extensions like .recipe.yaml and .recipe.plist may also help render the recipes properly in source control hosts and simplify methods people use to create and parse recipes outside of AutoPkg itself (e.g. CI/CD workflows).

However, there are some undesirable effects of this design.

First, AutoPkgr does not appear to detect .recipe.yaml files at this time. It's possible AutoPkgr could be updated to support this new extension, but their development resources are limited.

Of equal importance: branching the extensions supported by AutoPkg does inherently raise the cognitive load required to enter and understand the AutoPkg ecosystem, which may both discourage new users from using AutoPkg and generate additional support questions for those of us providing help to AutoPkg (and AutoPkgr) users.

Implementation

If the file extension requirements were reverted, here's one way it could happen:

  • A future version of AutoPkg would parse the ".recipe" extension multiple ways based on the contents of the file, allowing the recipe to be either plist or YAML.
  • Any new recipes or overrides created by AutoPkg would have the .recipe extension. The contents of the recipe/override would be formatted as desired (e.g. using autopkg new-recipe Foo.munki --format=yaml).
  • Existing recipes with the newly introduced .recipe.yaml and .recipe.plist extensions would continue being parsed and supported by AutoPkg for a period of time, but that support would be deprecated and ultimately removed.
  • AutoPkg's maintainers would help identify and update recipes with .yaml and .plist extensions within the AutoPkg org. Recipes outside the AutoPkg org would be the responsibility of their owners.

I'd like to keep this issue open for community feedback for at least 30 days before deciding to take any action.

@grahampugh
Copy link
Contributor

I don't really support this change or the reasons for it. File extensions are in most cases reflective of the code format used within them, for example .py, .sh, .swift and so on, and this is useful for identification, as well as for helping text editors/IDEs guess the code formatting.

Maybe I'm thinking too much as a writer of recipes rather than a consumer, but I'm not sure I agree that having the same extension for files which contain different formats within would be less confusing to people than the current status. I think it's helpful to people to know what the format is going to be without opening the file.

And it would surely be an easy fix for AutoPkgr to recognise ^.*\.recipe.[\.plist|\.yaml]$ instead of just ^.*\.recipe$ ?

I recognise that somebody decided to use .recipe when AutoPkg was designed, rather than using a format-based extension, but in my opinion adding .plist and .yaml has been an improvement. If it had always been .recipe.plist, text editors would have generally known what to do with it, and AutoPkgr would have been designed to handle it.

However, these are just my opinions, and of course if I have to remove all the extensions from my yaml recipes, I will.

Note: My plist-yaml-plist tool would need to be updated because it has logic in it to automatically add/remove .yaml when converting, and it does not expect to overwrite the source file with the converted file, so I would have to add a .bak or something to keep the source.

By the way, is it easy to programmatically detect that a file is yaml formatted, other than by extension? There's no shebang or other aid.

@gregneagle
Copy link
Contributor

The way you tell if a file is plist/yaml/json is you attempt to parse it as plist/yaml/json. If the yaml parser can successfully parse the file, it's a yaml file!

Just as a counter-point, Apple uses the .plist extension to identify Property List files. Property List files can be one of three formats: XML, binary, or old-style ASCII. The file extension does not tell you "in advance" which format the file is.

And yes, a binary-formatted plist can be a valid AutoPkg recipe...

@jazzace
Copy link
Member

jazzace commented Sep 9, 2021

I sit somewhere in the middle on this. I want the AutoPkg code to do the heavy lifting and be tolerant of reasonable choices. I would agree that the code should be able to parse any supported recipe format without the need for an extension (other than .recipe). Having said this, I believe it should tolerate .plist and .yaml as terminal extensions. I would not agree with dictating removal of such extensions for recipes in the repo. I can see arguments pro and con on this. A similar situation occurs with the .mobileconfig file format: it's a plist, but it can be signed or unsigned; the system needs to be able to deal with both, but you also don't use a .plist extension. I don't think there's a right answer. I think we can come up with a better answer than we currently have, however.

@grahampugh
Copy link
Contributor

The way you tell if a file is plist/yaml/json is you attempt to parse it as plist/yaml/json. If the yaml parser can successfully parse the file, it's a yaml file!

But very few code editing programs do this; they mostly go by extension. Again, I'm coming at this from a recipe writer's point of view, which may be the minority of AutoPkg users.

I remember when I still used to write plist recipes having to try and figure out how to edit Atom's config so that the markup highlighting would work for files with a .recipe extension. Because editing any sort of XML without colour syntax highlighting is not fun.

@gregneagle
Copy link
Contributor

I use TextMate. If I cannot figure out the text format (for highlighting) of a particular file you can tell it which one to use and it remembers it. Probably an advantage of using a Mac-specific text editor rather than a cross-platform one.

@nmcspadden
Copy link
Contributor

I also disagree - the file extension has never been the definitive guide to the contents of the file. It's a convention made to make things easier for humans. Text editors/IDEs are more than capable of opening up .recipe files and determining they are plists. There is no official ".recipe" file type, so no text editor will have automatic parsing or syntax highlighting for them, until you convince the editor/IDE that it's an XML plist. I can make a bash script called 'recipe.yaml' and bash will still execute it just fine. The contents are what matters, not the extension.

The fact that some recipes are in YAML is an implementation detail.

I agree that there should just be one file extension (".recipe"), and its contents should be agnostic. AutoPkg should try the ways it knows (i.e. "am I a plist?" or "is this yaml?") to read the file contents, and if it can't come up with a good understanding, skip the recipe (and hopefully inform the operator). In other words, humans shouldn't have to ever care whether a recipe is in YAML or Plist, or take any additional actions for it.

@grahampugh
Copy link
Contributor

It's a convention made to make things easier for humans.

This is my point exactly.

@nmcspadden
Copy link
Contributor

Rephrasing this another way: you can name your recipe whatever you want, but AutoPkg will only consider it for recipe evaluation if it ends in ".recipe". If you want to throw some other indicator that it's YAML in there, that probably won't interfere with anything, but if it doesn't end in ".recipe", AutoPkg shouldn't try to parse it as one. YAML recipes should not be treated as divergent from plist recipes in functionality, which is exactly what we're doing now for no reason.

In hindsight, I wish we had made this choice sooner, rather than letting it get here, but it was still new so I didn't think through the implications. But now I'm strongly of the opinion that diverging recipe types by filename is a bad idea.

@grahampugh
Copy link
Contributor

The way you tell if a file is plist/yaml/json is you attempt to parse it as plist/yaml/json. If the yaml parser can successfully parse the file, it's a yaml file!

AutoPkg will only consider it for recipe evaluation if it ends in ".recipe".

I don't want to labour the point, but these two statements seem to be at odds. If file determination should be done by parsing it, then why should AutoPkg care what the extension is? It should just look for a RecipeIdentifier key in the file (in a readable format), whatever it's called. The .recipe (or .recipe.yaml or .recipe.plist) is just to make things easier for humans, right?

@gregneagle
Copy link
Contributor

That would lead to autopkg attempting to parse every file in a directory: images, binaries, .DS_Store files, .py processors.

Limiting the attempted parsing to files ending in ".recipe" seems reasonable to avoid all that.

@jazzace
Copy link
Member

jazzace commented Sep 9, 2021

Limiting the attempted parsing to files ending in ".recipe" seems reasonable to avoid all that.

…or a finite set of extensions as is the case with the current version. Since AutoPkg allows shorthand (autopkg run Firefox.pkg for Firefox.pkg.recipe or Firefox.pkg.recipe.yaml), it is reasonable to have a finite set of extensions. I'm not certain I have been convinced that .recipe needs to be the only extension.

@gregneagle
Copy link
Contributor

gregneagle commented Sep 9, 2021

Right now we support ".recipe", ".recipe.plist" and ".recipe.yaml". There's someone hinting that JSON support is in the works so that would add ".recipe.json". In a few years there will be another serialization format that is the new hotness...

Again, I'd prefer to simplify matters by saying use ".recipe" as the extension if you want autopkg/AutoPkgr to recognize a file as a recipe inside a directory, which then allows for autopkg run Foo.munki. If you are running a recipe via full pathname, then the extension does not matter at all and can be literally anything (or nothing!)

One more point: when some people on this thread were maintaining both ".recipe" (plist) and ".recipe.yaml" versions of the same recipes, did you use different RecipeIdentifiers for each variant? If not, that means there were two recipes in your set of repos with the same identifier, which leads to undefined behavior (you cannot be certain which one autopkg might use).

@homebysix
Copy link
Member Author

One more point: when some people on this thread were maintaining both ".recipe" (plist) and ".recipe.yaml" versions of the same recipes, did you use different RecipeIdentifiers for each variant? If not, that means there were two recipes in your set of repos with the same identifier, which leads to undefined behavior (you cannot be certain which one autopkg might use).

The only repo that contained both plist and yaml versions of the same recipes (that I know of) is @grahampugh's, and he avoided identifier conflicts by putting the yaml recipes in a subfolder one level deeper than AutoPkg's search (for example: _YAML/Affinity/AffinityDesigner.pkg.recipe.yaml). More recently, the yaml recipes have replaced the plist recipes and moved out of the subfolder. I don't think there were duplicate identifiers in the search path at any point.

Duplicate identifiers can be a problem no matter which extension recipes use, and we already have methods for detecting them, so I'd classify that point as indirectly related.

@jazzace
Copy link
Member

jazzace commented Sep 9, 2021

To raise an additional question: Since we assume users read recipes in order to trust them, are we assuming that users are now bilingual? Having a visual cue in a file list that a recipe is not a plist would allow users to be monolingual if they so choose. That doesn't mean it has to be an extension, but there is benefit there to have a file naming convention of some sort. This would be of primary value to AutoPkgr users, who always add/override recipes from a list that only shows Recipe Name and Recipe Identifier. (I originally thought that naming the parent folder with YAML might be a solution, but the AutoPkgr use case makes that ineffective.)

@gregneagle
Copy link
Contributor

gregneagle commented Sep 9, 2021

The "bilingual" argument feeds into my general discomfort with supporting multiple recipe formats: it makes it twice as hard for users new to autopkg as they may have two new-to-them serialization formats to learn to read. (and if json is added: three)

And then consider the poor sap who has to understand a munki recipe in json format that has a parent pkg recipe in yaml format, which itself has a parent download recipe in plist format...

Perhaps we need a new autopkg verb that reads a recipe in whatever format it is and display it in a simpler, easy to human-parse format (which might be basically YAML)... but that wouldn't help people reading recipes on, say, GitHub.

@gregneagle
Copy link
Contributor

gregneagle commented Sep 9, 2021

It would also be possible to synthesize an "effective" recipe: This proposed autopkg verb could read the recipe and its parents (as it must do to run the recipe) and then display the effective "total" recipe.

@jazzace
Copy link
Member

jazzace commented Sep 9, 2021

I was originally thinking of a viewer app that could show side-by-side version syntax, but I like this new verb idea much better. The "recipe and its parents" synthesis would be of excellent value for debugging recipe chains during development. I like calling this verb display (since info is taken and list is ambiguous based on other AutoPkg uses). And I suspect @grahampugh would contribute his plist-yaml conversion code if we wanted the output to basically be YAML. I'm also thinking this idea is now a separate discussion. But very grateful we are having the discussion.

@nmcspadden
Copy link
Contributor

I don't want to turn this issue into a "should we support other structured data formats" debate, because that isn't really relevant here. YAML support exists and is here to stay, which means AutoPkg needs to understand it, and that means AutoPkg admins need to understand it too. I'm going to state a potentially controversial opinion that I don't think the requirement to be able to read YAML is an onerous burden to put on admins who are already required to read plists. I think you'd be hard-pressed to find anyone reasonably claiming YAML is harder than XML in terms of reading and writing, so I don't think this expectation is an unfair one. I also have no expectation or plan to support JSON or something like that because I think YAML satisfies pretty much all of the incentives for using something other than Plists.

The question for the issue isn't "is YAML fair to include in AutoPkg's ecosystem" - the question is whether YAML recipes should be required to have a different filename extension than plist recipes. To which I think the answer is still no. AutoPkg admins will have to accept that recipes can be in two different formats, and I don't think it's reasonable to expect to remain "monolingual."

There should be one recipe filename - ".recipe" and the fact that recipes could be in plist or YAML should be essentially a relatively unimportant detail.

I agree that there is effort that could be done to make viewing and understanding AutoPkg recipes easier, but no one has ever taken the time to really do this, and there hasn't been an ongoing demand for it beyond cursory "that would be a cool idea" discussions in Slack, so I'm not really certain how critical it is to get that going now. I'm happy to explore the idea with people interested in implementing it, but it's orthogonal to this discussion and I don't think it should be considered a blocker.

@gregneagle
Copy link
Contributor

I also cannot see a real need for .json recipes, but took the reaction to this message in MacAdmins Slack to mean someone was working on it:
image

@grahampugh
Copy link
Contributor

I also agree, I'm not sure what we would gain through json support, but maybe some people are most used to editing json that it is most comfortable for them. Especially AutoPkg for Windows people?

@homebysix
Copy link
Member Author

I'm not working on json support, nor am I interested in doing so. My reaction was a bit of a "more the merrier" joke, poorly executed.

@niloque
Copy link

niloque commented Sep 10, 2021

Hi, I'm no expert here, but why not just change the naming convention to: filename.plist.recipe / filename.yaml.recipe? We would leave the recipe extension as .recipe and admins would immediately see what is the format of the recipe file just by looking at the filename. Are there any unexpected implications of putting the 'real' extensions (plist/yaml) before the .recipe?

@grahampugh
Copy link
Contributor

@niloque Hmm, I'm not sure about that, since we are able to use things like autopkg search Firefox.jss to look for Firefox.jss.recipe*, I have a feeling that adding .yaml would break this.

@jazzace
Copy link
Member

jazzace commented Nov 10, 2021

After a lively discussion, this matter has sat for almost two months and there is no resolution in sight. Because @grahampugh has put a PR in to AutoPkgr to allow it to recognize YAML recipes, I'm going to suggest the following plan of action (or inaction, to be more accurate), which is fairly similar to what @homebysix laid out at the beginning of this thread:

  1. There will be no change to the requirement that YAML recipes have the .recipe.yaml extension.
  2. A PR/code change that would allow AutoPkg to correctly parse a YAML recipe that only had the .recipe extension in its name (i.e., remove the .recipe.yaml dependency) would be desirable and looked upon favourably by the maintainers.
  3. Should such a code change occur, we would revisit the question of whether the .recipe.yaml and .recipe.plist file extensions should be deprecated and eventually become unsupported.
    If this is where we are at, then it is useful to say so, since AutoPkgr users would benefit from Graham's PR.

@grahampugh
Copy link
Contributor

@jazzace unfortunately, my AutoPkgr PR does not solve the problem, according to the current maintainer. AutoPkgr also reads the contents of recipes by its own mechanism, to obtain Recipe Identifiers I suppose. So it also needs to understand the YAML recipes, not just know that they exist.

Therefore, it doesn't really matter whether AutoPkg supports YAML language recipes with the .recipe suffix or not, AutoPkgr still would need to be updated to support them.

@shawnhonsberger
Copy link

shawnhonsberger commented Nov 18, 2021

@jazzace As @grahampugh explained, the file extension is unfortunately not the core issue as I've tried this before. It is that AutoPkgr doesn't have the ability to parse YAML. In my initial rounds of research, there is not an elegant or simple way I've found to allow YAML parsing in ObjC without potentially affecting other aspects of AutoPkgr. AutoPkgr was built in such a way as to be dependent on XML, as that was presumably and reasonably assumed to remain the format of recipes at the time and going forward. I am still researching (in limited time) in how this can be done well. 😄

I will not say it is hopeless, and I will continue to still look into this, but I'm not yet confident this can be done - at least at this stage. I'm going to close Graham's PR for now. Thank you all for your patience with this! ❤️

@grahampugh
Copy link
Contributor

I'm kind of curious as to why AutoPkgr needs its own logic for parsing recipes, rather than using the output from AutoPkg itself. Does AutoPkg need a better API for this, for example plist outputs for search, info, make-override etc.?

@nmcspadden
Copy link
Contributor

nmcspadden commented Nov 19, 2021 via email

@homebysix
Copy link
Member Author

@grahampugh AutoPkgr parses plists in order to provide additional functionality like this without having to shell out to autopkg info for every right-click or table refresh.

image

@jazzace
Copy link
Member

jazzace commented Nov 19, 2021

This would explain why AutoPkgr doesn't always have the current info if you are operating in AutoPkgr and at the command line concurrently. (You have to quit AutoPkgr and relaunch in some cases for it to update.)

@jgstew
Copy link
Contributor

jgstew commented Aug 11, 2023

I'm late to this conversation, but all of my recipes are .recipe.yaml just due to the fact that then the IDE knows how to do syntax highlighting and parsing automatically and I kind of prefer to keep doing this going forward. The IDE keys off of the extension, as does yamllint in pre-commit, and prettier which I use to automatically format YAML files. I could configure each one of these things to tell them to automatically treat all .recipe files as YAML, but I would have to configure many things in many places to make that happen.

That said, I do think it is a good idea to allow .recipe to be any possible format (plist / yaml) if possible, even if I prefer to keep using .recipe.yaml myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants