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

RFC - Better Bolt/Puppet plan detection #129

Closed
glennsarti opened this issue May 24, 2019 · 5 comments
Closed

RFC - Better Bolt/Puppet plan detection #129

glennsarti opened this issue May 24, 2019 · 5 comments
Labels
enhancement New feature or request Language Server
Milestone

Comments

@glennsarti
Copy link
Contributor

glennsarti commented May 24, 2019

Problem

Bolt/Puppet plan files (*.pp manifest files) are raising linting/parsing errors. This is due to Pupet Editor Services (PES) not detecting it is a plan file and then not setting the Puppet parser into "tasks" mode.

Note that we can't force all puppet manifests into "tasks" mode as there are things you can't do in plans but you can in normal manifests and vice versa.

This RFC will describe how PES can reliably detect whether manifests are a plan or not.

Current Detection

The current detection is based on the documenation at https://puppet.com/docs/bolt/latest/writing_plans.html#concept-4485

  • Plans must be in a module, in the /plans directory
  • Modules must contain a metadata.json file

This is codified at https://github.com/lingua-pupuli/puppet-editor-services/blob/78bb4b108ed70b72ae48c08b56232579af36b92b/lib/puppet-languageserver/document_store.rb#L57-L64

Why the current detection fails

Bolt has introduced the concept of a Bolt project dir (or Boltdir) and this has caused issues like #127 and #128

https://puppet.com/docs/bolt/latest/bolt_project_directories.html

So now there are two aditional scenarios where plans can be located:

Note that in neither of these directory trees are a metadata.json or environment.conf so PES thinks that these .pp files are "standalone" and there is no workspace metadata information, therefore the linting is never put into "tasks" mode.

Local project directory

project/
├── Puppetfile
├── bolt.yaml
├── data
│   └── common.yaml
├── inventory.yaml
└── site-modules
    └── project
        ├── manifests
        │   └── my_class.pp
        ├── plans
        │   └── diagnose.pp  <---- THIS IS A PLAN
        └── tasks
            ├── init.json
            └── init.py

Embedded project directory

project/
├── Boltdir
│   ├── Puppetfile
│   ├── bolt.yaml
│   ├── data
│   │   └── common.yaml
│   ├── inventory.yaml
│   └── site-modules
│       └── project
│           ├── manifests
│           │   └── my_class.pp
│           ├── plans
│           │   └── diagnose.pp  <---- THIS IS A PLAN
│           └── tasks
│               ├── init.json
│               └── init.py
├── src #non Bolt source code for the project
└── tests #non Bolt tests for the project

Also note that plans can exist in subdirectories, for example project/Boltdir/site-modules/project/plans/foo/bar/wizz/diagnose.pp is a plan called project::foo::bar::wiz::diagnose

@glennsarti
Copy link
Contributor Author

glennsarti commented May 24, 2019

Plan Detection Solution 1

While naive, this will be effective.

"Given the full path to the .pp file, if it contains a directory called plans, AND that plans is not a sub-directory of manifests, then it is a plan file"

/project/Boltdir/site-modules/project/plans/foo/bar/wizz/diagnose.pp is a plan

something/plans/foo/bar/wizz/diagnose.pp is a plan

project/Boltdir/site-modules/project/manifests/init.pp is not a plan

project/Boltdir/site-modules/project/manifests/plans/init.pp is not a plan

Edit 1 : Updated with @genebean 's comments

@glennsarti
Copy link
Contributor Author

@genebean
Copy link
Contributor

Will that cause problems in a scenario where I have something like profile::app_name::plans::plan1 which would be located at profile/manifests/app_name/plans/plan1.pp?

Maybe there should be a conditional added to the "if path includes /plans" so that its actually "if path includes /plans and /plans is not a subfolder of /manifests"

@jpogran jpogran added enhancement New feature or request Language Server labels May 24, 2019
@glennsarti
Copy link
Contributor Author

Will that cause problems in a scenario where I have something like profile::app_name::plans::plan1 which would be located at profile/manifests/app_name/plans/plan1.pp?

Yup - thus the While naive comment.

Maybe there should be a conditional added to the "if path includes /plans" so that its actually "if path includes /plans and /plans is not a subfolder of /manifests"

👍

@glennsarti
Copy link
Contributor Author

glennsarti commented Jun 28, 2019

Closing and going with


"Given the full path to the .pp file, if it contains a directory called plans, AND that plans is not a sub-directory of manifests, then it is a plan file"

/project/Boltdir/site-modules/project/plans/foo/bar/wizz/diagnose.pp is a plan

something/plans/foo/bar/wizz/diagnose.pp is a plan

project/Boltdir/site-modules/project/manifests/init.pp is not a plan

project/Boltdir/site-modules/project/manifests/plans/init.pp is not a plan

@glennsarti glennsarti added this to the 0.20.0 milestone Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Language Server
Projects
None yet
Development

No branches or pull requests

3 participants