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

Use distgen to get distro and version combinations #75

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

mcyprian
Copy link
Contributor

@mcyprian mcyprian commented Apr 4, 2018

Distgen provides --multispec-combinations flag to get all valid (distro, version) combinations not excluded in multispec, This output can be used in generate.sh instead of guessing what distro should be used and rediscovering logic to exclude some of the combinations.
It was necessary to divide distgen rules into two categories, cases where only one file is rendered for each version independently from distros have to be handled separately. This PR fixes #74.

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@praiskup
Copy link
Contributor

praiskup commented Apr 4, 2018

Can you add Required-by: postgresql-container#<PR-NUMBER> into the commit message so we can jenkins-test against not-yet-merged postgresql-server container?

@praiskup
Copy link
Contributor

praiskup commented Apr 4, 2018

[test]

@mcyprian
Copy link
Contributor Author

mcyprian commented Apr 6, 2018

I am looking into CI build log, and I have no idea how could be this failure related to make generate rule I've made in this PR.

@pkubatrh
Copy link
Member

pkubatrh commented Apr 6, 2018

10:15:45 # FATAL: Can not be --ff merged into origin/master

Needs to be rebased

Isolate distgen multi rules that are used for each (distro, version)
combination not excluded in multispec, Use output of distgen's
--multispec-combinations flag to get the rigth versions and distros

Required-by: postgresql-container#266
@mcyprian
Copy link
Contributor Author

mcyprian commented Apr 6, 2018

Alright, thanks, rebased on top of current master branch.

@pkubatrh
Copy link
Member

pkubatrh commented Apr 6, 2018

[test]

@mcyprian
Copy link
Contributor Author

Can I please get some review or feedback to this? It is complicated to generate s2i-python-container files unless this is resolved.

@praiskup
Copy link
Contributor

I'm a bit lost where's the difference between DISTGEN_MULTI_RULES and DISTGEN_RULES.
Or rather, where's the difference between generating Dockerfile / and any other text file. Feel
free to grab me somewhere and explain f2f if its easier, maybe I'm missing something
(maybe docs are the problem?) dunno.

@mcyprian
Copy link
Contributor Author

mcyprian commented Apr 18, 2018

The main idea of this PR is to use output of dg --multispec specs/multispec.yml --multispec-combinations similar to this example

--distro centos-7-x86_64.yaml --multispec-selector version=3.6
--distro centos-7-x86_64.yaml --multispec-selector version=3.5
--distro centos-7-x86_64.yaml --multispec-selector version=3.4
--distro centos-7-x86_64.yaml --multispec-selector version=2.7
--distro fedora-27-x86_64.yaml --multispec-selector version=3.6
--distro rhel-7-x86_64.yaml --multispec-selector version=3.6
--distro rhel-7-x86_64.yaml --multispec-selector version=3.5
--distro rhel-7-x86_64.yaml --multispec-selector version=3.4
--distro rhel-7-x86_64.yaml --multispec-selector version=2.7

to determine what combinations of Dockerfiles will be generated. The current approach in generate.sh script can't handle the case when some (distro, version) combinations are excluded from the matrix.

At this moment the rules to generate Dockerfiles are mixed with rules to generate one copy of the file for each version directory like cccp.yml. In case we follow distgen multispec-combinations output, files like cccp.yml will be overwritten 4 times in some cases.

This is why I divided rules into two categories:

  • DISTGEN_RULES render one copy of the file for each version directory
  • DISTGEN_MULTI_RULES renders multiple files for each version directory - following multispec-combinations output.

@praiskup
Copy link
Contributor

praiskup commented Apr 18, 2018

Ok, I'm +1 to merge it here, just to unblock the ongoing work in python image,
etc. @pkubatrh wdyt?

Even though I have to say don't like the change (the same as I don't like the
manifest format and the overall generate.sh status before this PR). The
concrete problem here is that the new code expects that
Dockerfiles are somewhat special templates. And that's IMO clearly false
expectation, since we might well try to generate different README.md files for
centos-7-x86_4 and rhel-7-x86_64, same as other script files.

I don't want to block the PR here, as I did not before. It is not a big deal
since we could (if time permits) fix this in future, even if that required
rewrite it from scratch...

@mcyprian
Copy link
Contributor Author

@praiskup I see your points and I agree that manifest + generate.sh is not very understandable especially for someone looking into this for the first time and also not error proof (maybe some yaml file to declare rules and Python script to process it could work better). Your proposal #76 is nice and we might move this direction in the future. But these are all long time goals. At this moment Dockerfiles are special case so they probably need a special care as well.

@pkubatrh
Copy link
Member

I think it would be worthwhile to make the multirule parser generic enough so that it can be used on other files as well, as Pavel already suggested. Likely just checking the suffixes and defaulting to centos should be enough?

@pkubatrh
Copy link
Member

pkubatrh commented Apr 18, 2018

Actually now that I think about it again, those files would still have to retain the suffixes after being run through distgen so it is not the exact use case I had in mind there. I think it was something closer to already mentioned #76 which is something for the future.

@pkubatrh pkubatrh merged commit 2c28a31 into sclorg:master Apr 18, 2018
@pkubatrh
Copy link
Member

Merged as I did not want to block this anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to generate files for nondefault OS versions
4 participants