(trivial) Give notice about invalid modules

[radford]

Give notice when a module on the modulepath is invalid, explaining why.
To do this cleanly, we stop trying to add "." and ".." as modules since
these are always invalid.

[puppetcla]

Waiting for CLA signature by @radford

@radford - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

[puppetcla]

CLA signed by all contributors.

[kylog]

@andersonmills please take a look. Thanks!

[joshcooper]

Puppet.log_exception(e) is preferred so that if puppet is running with --trace it will show the backtrace leading up to the exception.

[kylog]

@radford thanks for the contribution! This makes a lot of sense. Can you add a ticket at tickets.puppetlabs.com and reference it in the commit message? (This doesn't meet our criteria for "trivial" since it does change behavior (in a good way) so it'll be nice to have it tracked as a ticket.)

Thanks!!

[hlindberg]

From triage:

• waiting on ticket to be created by @radford

[ffrank]

I'd be grateful if the commit message could give some more insight on how this end is achieved.
The diff itself is a little cryptic (to me, that is).

[radford]

Hi @ffrank and @kylog, I think I can address the triviality and the how at the same time. The patch adds a logging message, which is explicitly covered in TPE Policy and then filters "." and ".." which would otherwise lead to superfluous warnings but were ignored anyway beforehand. There is no practical change other than the addition of a useful warning.

[ffrank]

I'll admit that I'm a little edgy on the control flow change and I feel it warrants at least some explanation.

While you're at it, could you change the (trivial) tag to (maint)?

[radford]

@ffrank, along with changing the tag, I split the patch into three to provide separate motivations for each change.

[ffrank]

Awesome, thanks!

[andersonmills]

👍

[joshcooper]

@radford thanks for updating the PR. I have a few comments:

1. Can you update the commit message for radford@33c0571 to describe why Dir.entries.each is less efficient than Dir.foreach?
2. Looks like a typo in the above commit summary, should be "and less efficient"?
3. Can you add a test, at least for the case where an exception is raised and logged.

[radford]

@joshcooper, 1,2) I removed the Dir.foreach commit as it is orthogonal to the fundamental point of this PR. 3) I will not add a test that verifies that an exception is raised and logged. To check that the exception is logged would be to decide that catching this error and continuing is a good idea to begin with and I don't think it is.

[joshcooper]

@radford I don't follow your logic about, "to check that the exception is logged would be to decide that catching this error and continuing is a good idea to begin with and I don't think it is." We write tests to verify that the system behaves as expected, not whether or not we agree with the behavior.

[ffrank]

I think there is a misunderstanding here.

"Adding a test" refers to test cases in spec/ for TDD. The test is not supposed to happen in puppet code, which I believe @radford misconstrued.

[radford]

@ffrank, I understand what you were asking for in a test, but to me tests should check that something is happening the way that it should. @joshcooper, the question is what should happen in this code? What I haven't gleaned from any of your responses is what was this code trying to do? And what should it do?

• Should it be catching module exceptions to begin with?
• How many other different places does puppet traverse the module path? (lots)
  • And on how many of those does it check for invalid modules? (none)
  • So what happens when only this case skips invalid modules? (some of a module is loaded)
    • Why would running half a module be good idea? (it's not)

So, @joshcooper, what I'm saying is that I will not enshrine the current behavior in a test because I don't think it is doing the correct thing. I don't know what the correct thing is, but what I am certain about is that hiding the exception is wrong. To that effect this patch is a Band Aid that should be applied while someone thinks about the higher level questions.

[ffrank]

So you are saying that there is a higher level issue that this change will not solve?

Is there a ticket for that? If no, would you open one?

[joshcooper]

@radford I agree we should not be hiding the exception. Ensuring we log the exception sounds like a good first step, so I am 👍 for this change.

That said, this PR changes the behavior of puppet. Whenever there is a behavior change, we write tests to verify that puppet behaves the way we expect at that point in time. Writing a test doesn't "enshrine" the behavior -- it documents it, and we are free to change the behavior in the future. We simply update the code and the tests to match the new behavior.

Meanwhile, we should discuss what the new behavior should be on puppet-dev, or if you have ideas about what it should be, please file a ticket as @ffrank suggested, and write up how you think it should work.

The module loading code is invoked in many different contexts, especially with the new environments work going on between 3.5-3.7. I have not made any suggestions about how this should work, because I am not current on how environments interact with modules.

[hlindberg]

Triage Notes:

• @joshcooper to write a test and merge
• A ticket should be created for a more long term solution to the problem of "invalid modules"
• @hlindberg to add ticket

[joshcooper]

Btw, there is already a ticket filed about puppet's lack of debugging when a module has invalid metadata, https://tickets.puppetlabs.com/browse/PUP-3664