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

Ignores from lower layers can cause higher layer files to be ignored (dropped) #220

Closed
ryan-beisner opened this issue Jun 8, 2016 · 7 comments

Comments

@ryan-beisner
Copy link
Collaborator

commented Jun 8, 2016

I've found that not all files survive the charm build process. In this example, the layer I am building loses its README.md file from the "top" layer.

Repro

git clone https://github.com/openstack/charm-tempest
cd charm-tempest
tox -e build   # or charm build

Take note: README.md is here.

rbeisner@rbc:/tmp/charm-tempest$ ll
total 132
drwxrwxr-x 10 rbeisner rbeisner  4096 Jun  8 16:12 ./
drwxrwxrwt 17 root     root     16384 Jun  8 16:12 ../
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 actions/
-rw-rw-r--  1 rbeisner rbeisner   439 Jun  8 16:08 actions.yaml
-rw-rw-r--  1 rbeisner rbeisner  1620 Jun  8 16:08 config.yaml
-rw-rw-r--  1 rbeisner rbeisner   756 Jun  8 16:08 copyright
drwxrwxr-x  8 rbeisner rbeisner  4096 Jun  8 16:08 .git/
-rw-rw-r--  1 rbeisner rbeisner    57 Jun  8 16:08 .gitignore
-rw-rw-r--  1 rbeisner rbeisner    82 Jun  8 16:08 .gitreview
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 hooks/
-rw-rw-r--  1 rbeisner rbeisner 30425 Jun  8 16:08 icon.svg
-rw-rw-r--  1 rbeisner rbeisner   108 Jun  8 16:12 layer.yaml
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  8 16:08 lib/
-rw-rw-r--  1 rbeisner rbeisner   484 Jun  8 16:08 metadata.yaml
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 reactive/
-rw-rw-r--  1 rbeisner rbeisner   889 Jun  8 16:08 README.md
-rw-rw-r--  1 rbeisner rbeisner   438 Jun  8 16:08 requirements.txt
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 templates/
-rw-rw-r--  1 rbeisner rbeisner   331 Jun  8 16:08 .testr.conf
-rw-rw-r--  1 rbeisner rbeisner   288 Jun  8 16:08 test-requirements.txt
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 tests/
-rw-rw-r--  1 rbeisner rbeisner   959 Jun  8 16:08 tox.ini
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:08 unit_tests/
rbeisner@rbc:/tmp/charm-tempest$ cat layer.yaml 
includes: ['layer:openstack', 'interface:keystone-admin']
repo: https://github.com/openstack/charm-tempest
rbeisner@rbc:/tmp/charm-tempest$ charm build -o build
build: Destination charm directory: build/trusty/tempest
build: Processing layer: layer:openstack
build: Processing layer: layer:basic
build: Processing layer: tempest
build: Processing interface: keystone-admin

Take note: README.md did not survive to the built artifact.

rbeisner@rbc:/tmp/charm-tempest/build/trusty/tempest$ grep README .build.manifest 
    "hooks/relations/keystone-admin/README.md": [

rbeisner@rbc:/tmp/charm-tempest$ ll build/trusty/tempest/
total 136
drwxrwxr-x 11 rbeisner rbeisner  4096 Jun  8 16:14 ./
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  8 16:13 ../
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 actions/
-rw-rw-r--  1 rbeisner rbeisner   493 Jun  8 16:14 actions.yaml
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 bin/
-rw-rw-r--  1 rbeisner rbeisner 11951 Jun  8 16:14 .build.manifest
-rw-rw-r--  1 rbeisner rbeisner  2162 Jun  8 16:14 config.yaml
-rw-rw-r--  1 rbeisner rbeisner   756 Jun  8 16:08 copyright
-rw-rw-r--  1 rbeisner rbeisner    57 Jun  8 16:08 .gitignore
-rw-rw-r--  1 rbeisner rbeisner    82 Jun  8 16:08 .gitreview
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  8 16:14 hooks/
-rw-rw-r--  1 rbeisner rbeisner 30425 Jun  8 16:08 icon.svg
-rw-rw-r--  1 rbeisner rbeisner   322 Jun  8 16:14 layer.yaml
drwxrwxr-x  4 rbeisner rbeisner  4096 Jun  8 16:14 lib/
-rw-rw-r--  1 rbeisner rbeisner   518 Jun  8 16:13 Makefile
-rw-rw-r--  1 rbeisner rbeisner   525 Jun  8 16:14 metadata.yaml
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 reactive/
-rw-rw-r--  1 rbeisner rbeisner   438 Jun  8 16:08 requirements.txt
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  8 16:14 templates/
-rw-rw-r--  1 rbeisner rbeisner   331 Jun  8 16:08 .testr.conf
-rw-rw-r--  1 rbeisner rbeisner   288 Jun  8 16:08 test-requirements.txt
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 tests/
-rw-rw-r--  1 rbeisner rbeisner   959 Jun  8 16:08 tox.ini
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 unit_tests/
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  8 16:14 wheelhouse/

Versions (when using charm build)

rbeisner@rbc:/tmp/charm-tempest$ charm version
charm 2.1.1-0ubuntu1
charm-tools 2.1.2
rbeisner@rbc:/tmp/charm-tempest$ apt-cache policy charm
charm:
  Installed: 2.1.1-0ubuntu1
  Candidate: 2.1.1-0ubuntu1
  Version table:
 *** 2.1.1-0ubuntu1 500
        500 http://us.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages
        100 /var/lib/dpkg/status
rbeisner@rbc:/tmp/charm-tempest$ apt-cache policy charm-tools
charm-tools:
  Installed: 2.1.2-0ubuntu4
  Candidate: 2.1.2-0ubuntu4
  Version table:
 *** 2.1.2-0ubuntu4 500
        500 http://us.archive.ubuntu.com/ubuntu xenial/universe amd64 Packages
        500 http://us.archive.ubuntu.com/ubuntu xenial/universe i386 Packages
        100 /var/lib/dpkg/status

Versions (when using tox + venv)

FWIW, I see the same when using tox + venv + pip:

rbeisner@rbc:/tmp/charm-tempest$ . .tox/build/bin/activate
(build) rbeisner@rbc:/tmp/charm-tempest$ pip freeze | sort
appdirs==1.4.0
Babel==2.3.4
blessings==1.6
charm-tools==2.1.3
Cheetah==2.4.4
cliff==2.0.0
cmd2==0.6.8
colander==1.3.1
debtcollector==1.4.0
dnspython==1.14.0
ecdsa==0.13
funcsigs==1.0.2
functools32==3.2.3.post2
httplib2==0.9.2
iso8601==0.1.11
Jinja2==2.8
jsonschema==2.5.1
jujubundlelib==0.5.1
keyring==9.0
keystoneauth1==2.8.0
launchpadlib==1.10.3
lazr.authentication==0.1.3
lazr.restfulclient==0.13.1
lazr.uri==1.0.3
libcharmstore==0.0.3
Markdown==2.6.6
MarkupSafe==0.23
monotonic==1.1
msgpack-python==0.4.7
netaddr==0.7.18
netifaces==0.10.4
oauth==1.0.1
os-client-config==1.18.0
oslo.i18n==3.6.0
oslo.serialization==2.7.0
oslo.utils==3.11.0
otherstuf==1.1.0
paramiko==1.17.0
parse==1.6.6
path.py==8.2.1
pathspec==0.3.4
pbr==1.8.1
pkg-resources==0.0.0
positional==1.1.0
prettytable==0.7.2
psutil==1.2.1
pycrypto==2.6.1
pyparsing==2.1.4
python-neutronclient==4.2.0
pytz==2016.4
PyYAML==3.11
requests==2.10.0
requestsexceptions==1.1.3
ruamel.ordereddict==0.4.9
ruamel.yaml==0.11.11
simplejson==3.8.2
six==1.10.0
stevedore==1.14.0
stuf==0.9.16
testresources==2.0.0
theblues==0.3.1
translationstring==1.3
unicodecsv==0.14.1
virtualenv==15.0.2
wadllib==1.3.2
wrapt==1.10.8
wsgi-intercept==1.2.2
zope.interface==4.1.3

Checklist

  • Confirmed this is an issue with charm-tools, not charmstore-client
  • Provide versions of tools used
  • Described the feature or ways to replicate the issue

I expect/expected the following

Top layer root directory files should survive to the built charm.

What I got

Lost a file.

@ryan-beisner

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2016

Add'l info:

One of the lower layers [1] declares README.md ignored. The expected behavior is that the README.md file from only that layer is ignored. But it appears to be affecting even the top layer.

[1] https://github.com/openstack-charmers/charm-layer-openstack/blob/master/layer.yaml

includes: ['layer:basic']
ignore:
  - 'README.md'

@ryan-beisner ryan-beisner changed the title README.md not surviving charm build Ignores from lower layers can cause higher layer files to be ignored (dropped) Jun 9, 2016

@ryan-beisner

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2016

To check if this is resolved by d31304e, I re-built with requirements defined as `git+https://github.com/juju/charm-tools#egg=charm-tools`` (to pull in the committed fix from #218), but no love. The lower ignore prevails above still.

Just to confirm, base_layer.fetch() from that patch is present:

(build) rbeisner@rbc:/tmp/charm-tempest$ grep "    def fetch_dep(" -A 28 .tox/build/lib/python2.7/site-packages/charmtools/build/builder.py
    def fetch_dep(self, layer, results):
        # Recursively fetch and scan layers
        # This returns a plan for each file in the result
        baselayers = layer.config.get('includes', [])
        if not baselayers:
            # no deps, this is possible for any base
            # but questionable for the target
            return

        if isinstance(baselayers, str):
            baselayers = [baselayers]

        for base in baselayers:
            # The order of these commands is important. We only want to
            # fetch something if we haven't already fetched it.
            if base.startswith("interface:"):
                iface = Interface(base, self.deps)
                if iface.name in [i.name for i in results['interfaces']]:
                    continue
                results["interfaces"].append(iface.fetch())
            else:
                base_layer = Layer(base, self.deps)
                if base_layer.name in [i.name for i in results['layers']]:
                    continue
                base_layer.fetch()
                self.fetch_dep(base_layer, results)
                results["layers"].append(base_layer)

    def build_tactics(self, entry, current, config, output_files):

The README.md file is still dropped:

(build) rbeisner@rbc:/tmp/charm-tempest$ ll build/trusty/tempest/
total 160
drwxrwxr-x 12 rbeisner rbeisner  4096 Jun  9 08:47 ./
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  9 08:47 ../
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 actions/
-rw-rw-r--  1 rbeisner rbeisner   493 Jun  9 08:47 actions.yaml
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 bin/
-rw-rw-r--  1 rbeisner rbeisner 32641 Jun  9 08:47 .build.manifest
-rw-rw-r--  1 rbeisner rbeisner  2162 Jun  9 08:47 config.yaml
-rw-rw-r--  1 rbeisner rbeisner   756 Jun  8 16:08 copyright
-rw-rw-r--  1 rbeisner rbeisner    57 Jun  8 16:08 .gitignore
-rw-rw-r--  1 rbeisner rbeisner    82 Jun  8 16:08 .gitreview
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  9 08:47 hooks/
-rw-rw-r--  1 rbeisner rbeisner 30425 Jun  8 16:08 icon.svg
-rw-rw-r--  1 rbeisner rbeisner   322 Jun  9 08:47 layer.yaml
drwxrwxr-x  4 rbeisner rbeisner  4096 Jun  9 08:47 lib/
-rw-rw-r--  1 rbeisner rbeisner   518 Jun  9 08:47 Makefile
-rw-rw-r--  1 rbeisner rbeisner   525 Jun  9 08:47 metadata.yaml
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 reactive/
-rw-rw-r--  1 rbeisner rbeisner   495 Jun  9 08:46 requirements.txt
drwxrwxr-x  3 rbeisner rbeisner  4096 Jun  9 08:47 templates/
-rw-rw-r--  1 rbeisner rbeisner   331 Jun  8 16:08 .testr.conf
-rw-rw-r--  1 rbeisner rbeisner   288 Jun  8 16:08 test-requirements.txt
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 tests/
-rw-rw-r--  1 rbeisner rbeisner   959 Jun  8 16:08 tox.ini
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 unit_tests/
drwxrwxr-x  2 rbeisner rbeisner  4096 Jun  9 08:47 wheelhouse/

@marcoceppi marcoceppi added this to the 2.1.PATCH milestone Jun 9, 2016

@marcoceppi marcoceppi self-assigned this Jun 9, 2016

@marcoceppi

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2016

This is because a layer under the top layer has an

ignore:
  - 'README.md'

While the easy solution is to just remove that line, the ability to do an ignore per layer instead of globally is absolutely necessary. In addition, being able to ignore files from the top layer in lower layers seems equally as important. I'd like to propose the following.

First, that a new layer-ignore key is created, which will be a dict of layer names that are a list of files to ignore at that layer:

layer-ignore:
  layer-name:
    - 'FILE'
    - 'path/file'
  other-layer:
    - 'path'

The second change is that the current ignore key:

ignore:
  - 'README.md'

would be transformed to

layer-ignore:
  layer-name:
    - 'README.md'

Thoughts?

@ryan-beisner

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2016

I like the idea of having that level of granularity.

Another idea is for each layer to drop the listed files, but only for layers below it.

ryan-beisner added a commit to openstack-charmers/charm-layer-openstack that referenced this issue Jun 9, 2016
Stop ignoring README.md
Although that should be expected to work, the current charm
build behavior is that any ignore declarations at any layer,
both lower and higher levels, will be ignored during build.

That means that this declaration causes the top layer's
README.md to also be dropped and not included in the build.

This behavior is believed to be a bug, but pending resolution
within the tools, assume ignores are global to the whole
build stack.

Reference:  juju/charm-tools#220
@ryan-beisner

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 9, 2016

We worked around this unexpected behavior by removing the ignore declaration which caught this. But, we anticipate this will resurface in our needs to ignore the unit_tests and/or tests directories in lower layers.

@ryan-beisner

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2016

Although we worked around the one instance of this issue, it is still a blocker to certain aspects of our development in that we really do need to exclude unit_tests and other files from lower layers.

With the current "global" ignore behavior, the ignore feature is dangerous to use and difficult to troubleshoot because any arbitrary layer can cause any other layer's files to be dropped at build time.

@bcsaller

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

I think the direction this is headed is a good one.

The model is that for each layer from base to final we construct a set of files that will be in the final result. The base layer needs a way to say, "while this is in my repo don't include it in the set"

exclude: [ "unit_tests/"]

and the consumer (the layer above it) needs a way to say, I know I don't want this file from the base.

ignore: [ 'README.md'] ## though in this example it would just include its own overriding that

After that you are left with the set of all files except the ones the base didn't promote and the one that the consumer ignored.

If there is a layer above that one take the existing set and apply the rules again. The difference from today would be clearing the ignores like after each layer is processed and the addition of exclude, saying don't promote these files or directories.

How does this sound?

johnsca added a commit that referenced this issue Jul 17, 2016
Fix ignore and add exclude support in layer.yaml
Fixes #220 by making ignore apply only to the files provided by lower
layers and 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 johnsca referenced this issue Jul 17, 2016
4 of 4 tasks complete
johnsca added a commit that referenced this issue Jul 18, 2016
Fix ignore and add exclude support in layer.yaml
Fixes #220 by making ignore apply only to the files provided by lower
layers and 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 added a commit that referenced this issue Aug 24, 2016
Fix ignore and add exclude support in layer.yaml
Fixes #220 by making ignore apply only to the files provided by lower
layers and 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)
marcoceppi added a commit that referenced this issue Aug 24, 2016
#235: Fix ignore and add exclude support in layer.yaml (fixes #220 #234)
Fixes #220 by making ignore apply only to the files provided by lower
layers and 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.