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

content_sets (or content_sets_file) cannot be overridden #628

Closed
jmtd opened this issue Oct 24, 2019 · 6 comments · Fixed by #633

Comments

@jmtd
Copy link
Contributor

@jmtd jmtd commented Oct 24, 2019

cekit 3.5.0

With this image.yaml, using content_sets_file, and this override, using embedded content_sets, cekit crashes as follows

…
2019-10-24 13:41:03,773 cekit        DEBUG    Reading descriptor from 'image.yaml' file...
Traceback (most recent call last):
  File "/home/ce/cekit/3.5.0/bin/cekit", line 11, in <module>
    load_entry_point('cekit==3.5.0', 'console_scripts', 'cekit')()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/click/decorators.py", line 17, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 175, in build_osbs
    run_build(ctx, 'osbs')
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 278, in run_build
    run_command(ctx, builder_impl)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 247, in run_command
    Cekit(params).run(clazz)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/cli.py", line 337, in run
    command.execute()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/builder.py", line 53, in execute
    self.before_generate()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/builder.py", line 97, in before_generate
    self.generator.init()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/generator/osbs.py", line 21, in init
    super(OSBSGenerator, self).init()
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/generator/base.py", line 112, in init
    self.image.apply_image_overrides(self._overrides)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/descriptor/image.py", line 248, in apply_image_overrides
    override.packages.content_sets, self.packages.content_sets)
  File "/home/ce/cekit/3.5.0/lib/python2.7/site-packages/cekit/descriptor/base.py", line 183, in _merge_descriptors
    if k2 in desc1.skip_merging:
AttributeError: 'dict' object has no attribute 'skip_merging'

@jmtd

This comment has been minimized.

Copy link
Contributor Author

@jmtd jmtd commented Oct 24, 2019

Actually it's not that simple it seems. Switching to an external content_sets file in the override (here) and I get the same traceback. Likewise, only embeddeding the content_sets in image.yaml and then putting different embeds in the override, still breaks.

It seems impossible to override content sets.

@jmtd jmtd changed the title cekit crashes if override uses different approach for specifying content_sets content_sets (or content_sets_file) cannot be overridden Oct 24, 2019
@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Oct 24, 2019

Interesting, I'll take a look at it!

jmtd added a commit to jmtd/openjdk that referenced this issue Oct 24, 2019
We aren't gaining a lot from overrides, and I've hit some bugs trying
to override content_sets inherited from image.yaml (see
cekit/cekit#628). Instead, have four (+)
distinct, complete image descriptors, and name them after their
image names (except for the default, which has an old legacy name,
leave that as image.yaml).

Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
jmtd added a commit to jmtd/openjdk that referenced this issue Oct 24, 2019
We aren't gaining a lot from overrides, and I've hit some bugs trying
to override content_sets inherited from image.yaml (see
cekit/cekit#628). Instead, have four (+)
distinct, complete image descriptors, and name them after their
image names (except for the default, which has an old legacy name,
leave that as image.yaml).

Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
jmtd added a commit to jmtd/openjdk that referenced this issue Oct 24, 2019
We aren't gaining a lot from overrides, and I've hit some bugs trying
to override content_sets inherited from image.yaml (see
cekit/cekit#628). Instead, have four (+)
distinct, complete image descriptors, and name them after their
image names (except for the default, which has an old legacy name,
leave that as image.yaml).

Signed-off-by: Jonathan Dowland <jdowland@redhat.com>
@jmtd

This comment has been minimized.

Copy link
Contributor Author

@jmtd jmtd commented Oct 25, 2019

I've been thinking about this a little more. In the situation where the image.yaml uses (say) content_sets and the override uses content_sets_file (or the other way around), should that invalidate the content_sets key? It might be safer to check when both are being used and to bail out and say "don't do that"

@goldmann goldmann added this to To do in Release 3.6 via automation Oct 31, 2019
@goldmann goldmann added this to the 3.6.0 milestone Oct 31, 2019
@goldmann goldmann self-assigned this Oct 31, 2019
@goldmann goldmann moved this from To do to In progress in Release 3.6 Oct 31, 2019
@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 4, 2019

Internally, the content_sets_file key is converted into content_sets either case, so there shouldn't be any issue in handling both in overrides.

I'm working now on a fix for this and it looks like the merging approach could be pretty simplified. I would like to ask you to test the upcoming release once I have something ready. This would give me more confidence in the release. All tests are passing, but I would like to have more real-world use cases be tested as well.

goldmann added a commit to goldmann/cekit that referenced this issue Nov 4, 2019
This rewrites how overrides are handled for content sets.
From now all the work to merge packages is offloaded to the merge
algorithm.

Fixes cekit#628
goldmann added a commit to goldmann/cekit that referenced this issue Nov 4, 2019
This rewrites how overrides are handled for content sets.
From now all the work to merge packages is offloaded to the merge
algorithm.

Fixes cekit#628
@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 4, 2019

@jmtd Can I ask you to help me test this patch?

pip install -U https://github.com/goldmann/cekit/archive/gh-628-overriding-content-sets.zip
@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 4, 2019

@errantepiphany @luck3y Can I ask you to do the same with some fun images? I would like to make sure it doesn't break anything for you.

@goldmann goldmann closed this in #633 Nov 6, 2019
Release 3.6 automation moved this from In progress to Done Nov 6, 2019
goldmann added a commit that referenced this issue Nov 6, 2019
This rewrites how overrides are handled for content sets.
From now all the work to merge packages is offloaded to the merge
algorithm.

Fixes #628
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release 3.6
  
Done
2 participants
You can’t perform that action at this time.