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

Generate workflow files with a Jinja template #2687

Merged
merged 11 commits into from
Aug 6, 2024

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jul 10, 2024

Fixes #2686

The idea behind this PR is to delete our current test and lint workflow files and replace them with automatically generated workflow files.

This automatically generated file gets its information directly from tox.ini, removing the duplication of test running information in tox.ini and in the Github workflow files.

To achieve this, I have added a Jinja2 template that is used to generate the new workflow file. The resulting workflow file does not use a Github matrix. This is intentional, a Github matrix only supports 256 jobs, which had forced us to have 2 almost identical workflow files. With this new approach we only need one huge file.

I also cleaned up the information that is displayed in the web browser, so jobs now have more user-friendly names.

I also added a check that makes sure that the automatically generated workflow file is up to date with every PR that modifies the automatically generated workflow files so that we don't miss adding any necessary changes to this file.

I also removed the step where we cache the tox environment. Since every tox run happens in a separate runner, I think there is no point in saving this cache.

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 10, 2024
@ocelotl ocelotl requested a review from a team July 10, 2024 02:00
@ocelotl ocelotl self-assigned this Jul 10, 2024
@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 10, 2024

We need the same for lint, I'll do that in a new PR.

tox.ini Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@ocelotl ocelotl force-pushed the issue_2686 branch 3 times, most recently from b092501 to e39b90f Compare July 10, 2024 23:00
@ocelotl ocelotl force-pushed the issue_2686 branch 2 times, most recently from 4cb0112 to 78fdff2 Compare July 15, 2024 20:55
@ocelotl ocelotl changed the title Remove duplication of test running information Generate workflow files with a Jinja template Jul 15, 2024
@ocelotl ocelotl force-pushed the issue_2686 branch 4 times, most recently from 63c887f to 3511ad6 Compare July 16, 2024 20:47
@github-actions github-actions bot requested a review from codeboten July 16, 2024 22:31
@ocelotl ocelotl force-pushed the issue_2686 branch 5 times, most recently from ac327bc to 9940f09 Compare July 17, 2024 01:58
@ocelotl ocelotl closed this Jul 24, 2024
@ocelotl ocelotl reopened this Jul 24, 2024
@ocelotl ocelotl force-pushed the issue_2686 branch 3 times, most recently from 104867d to b7d6b3e Compare August 1, 2024 19:16
@ocelotl
Copy link
Contributor Author

ocelotl commented Aug 1, 2024

Bit of nit/soft comment for consideration: it looks like you can use a github workflow to generate the matrix via JSON: kenmuse.com/blog/dynamic-build-matrices-in-github-actions

This would avoid having a separate template with text munging and the script you added could instead output JSON. A lot of what the template does is "reimplementing" GHA features like variable interpolation and matrices. Any thoughts?

Ah, yes, I remembered this when I saw this uses $GITHUB_OUTPUT. This was actually my first approach, I tried and then moved a way from this approach. What I realized was that the matrix approach has a few disadvantages:

  1. Need to produce a exclude section: so, the tox.ini file says which python versions are used for every package. It doesn't say which python versions are not used for every package. We could of course produce an exclude section based on this information but I find it cleaner to only produce jobs that are to be run instead of "producing" them all and then removing the ones that are not to be executed.
  2. No fine control on the workflow files: Sometimes we don't want to follow the exact same steps for every job. For example, in the core repo, we have a step that only gets executed if the underlying OS is Windows (configuring git to allow for long file names). We are actually running this step in Ubuntu, just skipping its contents. It is cleaner not to have it at all if the OS is Ubuntu, a Jinja template allows us to do that, a matrix doesn't. Also, we are using templates in this PR for consistency for all the jobs (lint, docker tests, etc) and in some of those jobs we also need the template to do something particular for different jobs, so we need templates for this reason too.
  3. The result is obvious. Once the (huge, yes) workflow file is created, it is completely obvious for whoever reads it what is happening in CI, there is no need to figure out the matrix. This approach requires the workflow files to be generated by hand in some PRs that will introduce things like new packages or adding support for a python version for a particular package, etc. So, I added a check that works in the same way as the one we already have for tox -e generate, which will fail CI if the user has not submitted the generated workflow files in the PR.

@ocelotl ocelotl requested a review from xrmx August 1, 2024 19:35
@ocelotl ocelotl requested a review from emdneto August 1, 2024 23:41
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sympathetic to Aaron concerns regarding the obvious huge listing vs the compactness of the gh matrix but once we are generating the workflows if we see areas of improvements we can iterate on it.

@ocelotl ocelotl force-pushed the issue_2686 branch 2 times, most recently from b9f3b44 to 21c624d Compare August 5, 2024 16:51
@ocelotl ocelotl merged commit f0d8cb3 into open-telemetry:main Aug 6, 2024
616 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate test running information in tox.ini and CI workflows
5 participants