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

(GH-2438) Don't warn that project content won't be loaded if there's no project content #2468

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

lucywyman
Copy link
Contributor

This limits the warning that project content won't be loaded if the
project doesn't have a name to only be raised if the project has a
tasks/, plans/, or files/ directory.

!bug

  • Only warn that project content won't be loaded if there's project content

    Bolt will now only warn that project content won't be loaded if the
    proejct directory has a tasks/, plans/, or files/ directory that
    may contain content.

@lucywyman lucywyman requested a review from a team as a code owner December 11, 2020 22:00
…f there's no project content

This limits the warning that project content won't be loaded if the
project doesn't have a name to only be raised if the project has a
`tasks/`, `plans/`, or `files/` directory.

!bug

* **Only warn that project content won't be loaded if there's project content**

  Bolt will now only warn that project content won't be loaded if the
  proejct directory has a `tasks/`, `plans/`, or `files/` directory that
  may contain content.
elsif name.nil? &&
(File.directory?(plans_path) ||
File.directory?(@path + 'tasks') ||
File.directory?(@path + 'files'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we including the files/ directory in this check? I know tasks/ and plans/ make sense, since you can reference content in those directories from the CLI, but I don't think you can for files/ (unless I'm missing a command).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could reference a file from the project in a manifest being applied, for (very contrived) example:

bolt apply -e " file { '/tmp/foobar': content => file('myproject/service.conf'), ensure => present }"

Or more likely from a manifest file you specify with bolt apply.

I think we opted not to include manifests for the reason you said, that usually if you're consuming manifests it's probably in a plan (in which case you'd get the error). We could add manifests/ too to err on the side of zealously warning, in case people are consuming the project from a module on the modulepath or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really worried about it, and it's easy enough to add to the check later if needed. :) I was more just curious why files/ was included, and that example makes perfect sense!

@beechtom beechtom merged commit cf314a9 into puppetlabs:main Dec 14, 2020
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

Successfully merging this pull request may close these issues.

Don't print no project name warning when bolt-project.yaml doesn't exist
2 participants