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

osbs configuration is overridden when using --redhat #631

Closed
hmlnarik opened this issue Oct 31, 2019 · 4 comments · Fixed by #635

Comments

@hmlnarik
Copy link

@hmlnarik hmlnarik commented Oct 31, 2019

When initiating a build that uses --redhat defaults and overrides image.yaml osbs entry with a YAML passed in via one of the --override options, the osbs entry is ignored.

To reproduce
Have my-overrides.yaml file that contains osbs / configuration / container entry. Then run:

cekit --redhat build--overrides-file my-overrides.yaml osbs

Expected behavior
container.yaml is generated.

Logs
Paste any logs you have, use CEKit verbose output: cekit -v.

Environment information:

  • Operating system: Fedora
  • CEKit version: 3.5.0

Additional context
Reason is that the RedHatOverrides include osbs key which is then applied as the last override. This completely removes custom osbs configuration.

@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 4, 2019

Hm, you say that RedHatOverrides mangles the osbs section, but in fact it does not touch it at all. What RedHatOverrides does is to add specific environment variables and labels.

I would need to work on a reproducer first, but it's hard since I do not entirely understand the issue. Similar command specified as a reproducer is used by many teams already so it's even harder to understand the report. Can you show a working example where it does not work as expected?

@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 5, 2019

OK, I have dig a bit more into this issue and it looks like it is in fact a bug. Most probably it was introduced by this commit: b63e952 and the changes made to the cekit/descriptor/image.py file. I'm looking now what is the root cause of the problem, I suspect it is related to the how the merge code works with descriptors.

@goldmann goldmann added this to To do in Release 3.6 via automation Nov 5, 2019
@goldmann goldmann added this to the 3.6.0 milestone Nov 5, 2019
goldmann added a commit to goldmann/cekit that referenced this issue Nov 5, 2019
Make sure that the osbs section can be correctly merged with overrides.
In some cases the container.yaml file was not generated to incorrect
handling of merging.

Fixes cekit#631
@goldmann

This comment has been minimized.

Copy link
Contributor

@goldmann goldmann commented Nov 5, 2019

@hmlnarik I've opened another PR that takes care of the root cause of this issue: #635

goldmann added a commit to goldmann/cekit that referenced this issue Nov 5, 2019
Make sure that the osbs section can be correctly merged with overrides.
In some cases the container.yaml file was not generated to incorrect
handling of merging.

Fixes cekit#631
@hmlnarik

This comment has been minimized.

Copy link
Author

@hmlnarik hmlnarik commented Nov 5, 2019

Thanks @goldmann , I meditated over removing that line with skip_merging as well but did have time to check all potential consequences of such a change and thus decided for a more conservative fix proposed in #632. Feel free to close that one, and thanks for fixing this bug!

@goldmann goldmann closed this in #635 Nov 5, 2019
Release 3.6 automation moved this from To do to Done Nov 5, 2019
goldmann added a commit that referenced this issue Nov 5, 2019
Make sure that the osbs section can be correctly merged with overrides.
In some cases the container.yaml file was not generated to incorrect
handling of merging.

Fixes #631
@goldmann goldmann self-assigned this Nov 5, 2019
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.