Fix ignore and add exclude support in layer.yaml

[johnsca]

Description of change

Checklist

• Have you followed Juju Solutions hacking guide?
• Are all your commits [logically] grouped and squashed appropriately?
• Does this patch have code coverage?
• Does your code pass make test?

Fixes #220 by making ignore apply only to the files provided by layers lower than the one currently being processed, and by adding an exclude counterpart which applies only to the current layer.

Fixes #234 by refactoring ignore support and making it an error for no tactics to match a file (there should always be a fall-back, such as CopyTactic).

[johnsca]

Note: To make the IgnoreTactic work, the trigger method signature needed to be modified. I'm fairly sure we don't have any custom tactics in the wild, but if we do, this could potentially break them.

[mbruzek]

You don't check if tactic is None here. Are you sure tactic can never be None?

[johnsca]

candidate is a class not a function, so it is guaranteed to return an instance (this is basically like IgnoreTactic(entity, target, layer, next_config)).

[mbruzek]

@johnsca we discussed the two ways charm-tools currently excluded files. I don't see the removal of the existing ignore code. Did I miss it somewhere in here?

[mbruzek]

OK I see the old ignore code removed here.

[mbruzek]

The old ignore code is removed here.

[mbruzek]

I have reviewed this code and it looks sound to me. +1

[bcsaller]

Why an ignore tactic at all? In the BulidConfig.tactic method you could return None if the current entity is in the excludes list which won't include a tactic at all. This would be almost identical to the handling of the ignore list there which returns None if next layer would ignore this file.

Its is very possible I am missing something, but at a glance

def tactic(self, entity, current, target, next_config):
        # Produce a tactic for the entity in question
        # These will be accumulate through the layers
        # and executed later
        bd = current.directory
        spec = pathspec.PathSpec.from_lines(pathspec.GitIgnorePattern,
                                            self.excludes)
        p = entity.relpath(bd)
        matches = spec.match_files((p,))
        if p in matches:
            return None

        # Ignore handling
        if next_config:
            spec = pathspec.PathSpec.from_lines(pathspec.GitIgnorePattern,
                                                next_config.ignores)
            p = entity.relpath(bd)
            matches = spec.match_files((p,))
            if p in matches:
                return None

        for tactic in self.tactics():
            if tactic.trigger(entity.relpath(bd)):
                return tactic(target=target, entity=entity,
                              current=current, config=next_config)
        return None

seems simpler

Here I include the default ignore list as well, but really that is just saying by default we don't expect our .git/.bzr stuff to make it into the output.

[mbruzek]

@bcsaller This actually came up with Issue #234 where I was able to find a case where the current tactic came back as None and this line did not handle it well: https://github.com/juju/charm-tools/blob/master/charmtools/build/builder.py#L298

Using a debugger I was able to find many cases where both current and existing were None, but the case where existing was an object and current was None that caused the traceback in the issue.

I added a check for current not None to the if statement and talked with @johnsca about this. We felt that having None as a tactic would mean that there is no way to handle the file. Combined with the feature request to tie ignored files to layers it made sense to use to add an ignore tactic.

[bcsaller]

I might be misunderstanding something. I mean that if tactic() returns None
it is ignored in the planning phase, see Builder.plan_layers where None
tactics are ignored. This means that the current way to ignore something is
to produce no tactic for it rather than add a new type which does nothing.
shrug

On Mon, Jul 18, 2016 at 9:38 AM Matt Bruzek notifications@github.com
wrote:

@bcsaller https://github.com/bcsaller This actually came up with Issue
#234 #234 where I was able to
find a case where the current tactic came back as None.
https://github.com/juju/charm-tools/blob/master/charmtools/build/builder.py#L298

Using a debugger I was able to find many cases where current and existing
were None, but the case where existing was an object and current was None
that caused the traceback in the issue.

I added a check for current being None to the if statement and talked with
@johnsca https://github.com/johnsca about this, he felt that having a
None tactic was having no way to handle the file and felt that was not the
best way to handle the ignore files.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#235 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGVheRpezXrfP3eh0wtffRg2xvILTk-ks5qW6wBgaJpZM4JOS6-
.

[johnsca]

@bcsaller Your suggestion is pretty much identical to the existing implementation which suffers from both bugs.

We don't want tactic() to return None because that ignores the file from the current layer in addition to all below. Instead, we need to return the correct tactic for the file, but disregard any previous tactic. There is no way to say "ignore the file from the layers below me, but use the ones I provide" with your implementation (this matters for files that would be merged, or for directories where you want to say, e.g., "ignore all of the tests provided by lower layers, but include the tests I provide"). Further, returning None from tactic() is indistinguishable from having no tactics match (i.e., we don't know how to handle the file) which is something that we should catch as an error. Also, a None result wasn't handled in plan_layers for all cases because it was blowing up in build_tactics, hence #234.

Your suggestion also doesn't address the issue that ignores (and also now excludes) would be propagated up through any layer that doesn't specify a value, though that only requires the new implementations of the ignore and excludes properties.

The main reason I went with IgnoreTactic and ExcludeTactic was that it seemed like a better design. It encapsulates the logic for those rules and makes them tactics like anything else, instead of being special-case logic.

This is by no means the smallest nor least invasive change that could have addressed these issues, but it seemed like a reasonable refactor. However, it does change the signature of the Tactic methods, as I mentioned, so if that doesn't seem worth this refactor, if it is disagreed that this refactor is an improvement, or if @marcoceppi is uncomfortable with it for a bug-fix release, I can come up with an alternative with a more minimal diff.

[johnsca]

btw, the DEFAULT_IGNORES were supposed to be included in both ignores and excludes, and were left out by mistake. I will update the commit to restore them.

[johnsca]

Commit updated to include DEFAULT_IGNORES.

[bcsaller]

This makes more sense now, thanks for the explanation.

[johnsca]

Rebased to master. Would like to get this merged and included with the next release.

[marcoceppi]

Not sure how this one missed the ⛵️ but it's on it now