sclorg / container-common-scripts

Apache License 2.0
21 stars 45 forks source link

New generator written in Python #294

Closed frenzymadness closed 1 year ago

frenzymadness commented 1 year ago

This replaces the black Makefile magic and the complicated shell generator with a much simpler Python script. I'm gonna open pull requests for container images where we use distgen to show the results the new generator provides.

frenzymadness commented 1 year ago

Python: https://github.com/sclorg/s2i-python-container/pull/545 Postgre: https://github.com/sclorg/postgresql-container/pull/465 Varnish: https://github.com/sclorg/varnish-container/pull/105

This PR has to be merged first and then we can update the submodule in the other repositories. [test]

frenzymadness commented 1 year ago

It looks good to me. But would it be possible to add some test suite?

Do you mean some unit tests for the functions in the generator module? Or do you want something more complex?

phracek commented 1 year ago

@frenzymadness I have tried to run the new generate on PR https://github.com/sclorg/s2i-python-container/pull/546 by command make generate-new and the result of git status is:

    deleted:    src/rhel
    typechange: test/check_imagestreams.py
    deleted:    test/imagestreams
    typechange: test/test-lib-openshift.sh
    typechange: test/test-lib.sh
    typechange: test/test-openshift.yaml

The original is:

$ ls -la src/rhel
lrwxr-xr-x  1 phracek  staff  6 Jan  6  2022 src/rhel -> centos
$ ls -la test/imagestreams
lrwxr-xr-x  1 phracek  staff  16 Jan  6  2022 test/imagestreams -> ../imagestreams/
$

It looks like the symlinks disappeared.

phracek commented 1 year ago

I would say everything ;) but we can start with unit tests :)

frenzymadness commented 1 year ago

@frenzymadness I have tried to run the new generate on PR sclorg/s2i-python-container#546 by command make generate-new and the result of git status is:

  deleted:    src/rhel
  typechange: test/check_imagestreams.py
  deleted:    test/imagestreams
  typechange: test/test-lib-openshift.sh
  typechange: test/test-lib.sh
  typechange: test/test-openshift.yaml

The original is:

$ ls -la src/rhel
lrwxr-xr-x  1 phracek  staff  6 Jan  6  2022 src/rhel -> centos
$ ls -la test/imagestreams
lrwxr-xr-x  1 phracek  staff  16 Jan  6  2022 test/imagestreams -> ../imagestreams/
$

It looks like the symlinks disappeared.

I cannot reproduce it. I have fetched the branch you have opened the PR from (support_cvp_pipeline) and ran make generate-new and there is nothing in git status. The generator should change nothing in src and test folders so there is something strange happening on your machine. Could you please paste the full output of the command here?

phracek commented 1 year ago

I have copied this PR to s2i-python-container and sources are generated properly. I forgot to copy your pull request. Sorry.

phracek commented 1 year ago

@frenzymadness Please rebase it against the master

frenzymadness commented 1 year ago

Rebased

zmiklank commented 1 year ago

[test-all]

zmiklank commented 1 year ago

@frenzymadness could you please look at the tests results for RHEL8, postresql-container[1]?

The generator fails on invalid syntax:

common/generator.py -v $version -m manifest.yml -s specs/multispec.yml ; \
fi \
done
  File "common/generator.py", line 70
    if m := re.match(r".*\.rhel(\d+)$", filename):
          ^
SyntaxError: invalid syntax

[1] http://artifacts.osci.redhat.com/testing-farm/dbf739df-b662-460b-bfec-7d80a4d4d2df/

frenzymadness commented 1 year ago

@frenzymadness could you please look at the tests results for RHEL8, postresql-container[1]?

The generator fails on invalid syntax:

Yeah, thanks, I have to fix that. The walrus operator is available in Python 3.8+ but the default Python in RHEL 8 is 3.6.

frenzymadness commented 1 year ago

[test-all]

frenzymadness commented 1 year ago

I'm investigating Testing Farm - RHEL8 - postgresql-container – the error says ModuleNotFoundError: No module named 'yaml' which is strange because distgen itself depends on yaml (and imports it) so if the distgen is there (which should be, right?) then an importable yaml module should be there as well. But I have no idea how is the distgen installed there.

frenzymadness commented 1 year ago

It seems that the result (the log) of Python container on RHEL 8 is incomplete.

zmiklank commented 1 year ago

It seems that the result (the log) of Python container on RHEL 8 is incomplete.

The full log can be found here[1]. Testing Farm prints only first n lines of log output to the main webpage with results. But there is always a link to the directory with all test artifacts on the upper part of the webpage.

[1] http://artifacts.osci.redhat.com/testing-farm/37b83134-0a4f-4572-857f-e51e5f839493/work-rhel8-dockerMHFq4D/log.txt

zmiklank commented 1 year ago

I'm investigating Testing Farm - RHEL8 - postgresql-container – the error says ModuleNotFoundError: No module named 'yaml' which is strange because distgen itself depends on yaml (and imports it) so if the distgen is there (which should be, right?) then an importable yaml module should be there as well. But I have no idea how is the distgen installed there.

Installation of distgen (and other needed packages) is done by the TMT (from the TMT plan). The package should be installed from Fedora or epel repositories. I am however not sure, what can be the source of this error. I will need to take closer look.

frenzymadness commented 1 year ago

Installation of distgen (and other needed packages) is done by the TMT (from the TMT plan). The package should be installed from Fedora or epel repositories. I am however not sure, what can be the source of this error. I will need to take closer look.

Thank you. My assumption was that distgen depends on pyyaml so it's safe to use it also for the new generator.

zmiklank commented 1 year ago

[test-all]

frenzymadness commented 1 year ago

Just to summarize new discoveries and what we agreed on at a meeting last week.

The current situation is that some container images regenerate versioned folders before the build phase. That's the reason we need to install distgen everywhere. The problems are:

  1. the new generator is not compatible with Python 2 and therefore does not work on Centos 7 (where distgen is built for Python 2 only)
  2. the results of the generator are exactly the same no matter the platform it runs on so it does not make sense to spend additional time installing and running it everywhere.
  3. the generator (make generate) is not used everywhere by default but it depends on a container image itself. For example Python container image does not regenerate its content before built, but in postgres it's implemented in a hacky way by overloading a make target from common scripts. Nonetheless, it's a bad idea to regenerate versioned folders before build because then what we ship to registries is different from what we have in git repositories.

We have agreed that nobody will really use the generator on Python 2 and if a request to support Centos 7 appears, we can either provide a workaround (install distgen into a virtual env for Python 3) or we can start building distgen for Python 3 in EPEL.

The plan is to remove the hacks from postgre and varnish and introduce a new step into GH actions. The new step will do the following:

  1. Run make generate
  2. Is there any difference between the generated content and what is in the repository?
    • if yes, the tests are stopped immediately
    • if not and the generated sources are the same, run all the testing jobs on the testing farm as usual.

Cc @zmiklank @phracek @pkubatrh

frenzymadness commented 1 year ago

To be more specific, we can follow these steps:

  1. merge this PR. It'll break CI but only for a while.
  2. update common submodule in the linked PRs
  3. review the linked PRs - they contain some fixes and changes related to this so a review is needed for all of them.
  4. merge the linked PRs - this will fix the CI here because distgen won't be needed in the common CI anymore.
  5. implement the new check in GH actions for Python, Postgres, and Varnish. It can be kinda isolated as it doesn't need a testing farm because all we need should be available in every container with Python 3.

I'm not sure we'll be able to prepare and test everything in advance so I'd not wait for all PRs to be ready.

frenzymadness commented 1 year ago

@zmiklank @phracek @pkubatrh what do you think about my plan?

zmiklank commented 1 year ago

Oh yes, I agree

@phracek how far we are with https://github.com/sclorg/container-common-scripts/pull/275? Do you think we should use this PR as a check within the GitHub Action, or is better, from your point of view, to write something new? If you do not have time for that I could do that.

frenzymadness commented 1 year ago

I'm gonna proceed with the plan now. I'll merge this, update common submodules and merge the linked PRs in the container images' repos.