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

Enable collection integration tests on GHA #14397

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

relrod
Copy link
Member

@relrod relrod commented Aug 30, 2023

SUMMARY

There are a number of changes here:

  • Abstract out a GHA composite action for running the dev environment
  • Update the e2e tests to use that new abstracted action
  • Introduce a new (matrixed) job for running collection integration tests. This splits the jobs up based on filename.
  • Collect coverage info and generate an html report that people can download easily to see collection coverage info.

NOTE We should make the new checks be non-voting for a week or two, and make sure they don't have intermittent flakes.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • Collection

@github-actions github-actions bot added the component:awx_collection issues related to the collection for controlling AWX label Aug 30, 2023

- uses: ./.github/actions/upload_awx_devel_logs

collection-integration-coverage-combine:
Copy link
Member

Choose a reason for hiding this comment

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

I see that the previous job produces coverage data... what is this combining with?

Copy link
Member Author

Choose a reason for hiding this comment

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

So:

  • The matrix jobs will each produce a bunch of coverage files.
  • They upload them as an artifact (I wish there were a better way to do that, I don't really want those to stay around as artifacts, they aren't very useful to most people... but we need them all to generate a report.)
  • Then this job grabs all the coverage files from all the matrix jobs, shoves them into a place where ansible-test knows how to work with them, and then asks ansible-test to combine them into a single file and generate a report from that file.

It kind of has to be done this way because pycoverage has to know where to look for the actual source code to include it in the reports. And ansible-test does some hackery to make that all work out nicely for collections.

run: |
echo "${{ inputs.github-token }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin

- name: Pre-pull image to warm build cache
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're just running out of that image, not just warming up your cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

That comment/name came from the original e2e_tests, IIRC. But yeah, valid, I can change it.

Actually, I think it'd be really cool to cache the database every once in a while. Then just let things fetch a relatively recent database and apply the latest few migrations on top of it.

But that's a rainy-day idea.

Copy link
Member Author

@relrod relrod Aug 31, 2023

Choose a reason for hiding this comment

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

So I've updated it so that it builds the image each time now from the current checkout.

I guess we do want that, even here for the collection tests (consider: developing a feature that also has collection changes, we want to test them in lockstep). But since it adds so much time, I might split up the job matrix a bit more to try to get the time back down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking more on this and doing some more research -- we're using buildkit's inline caching which only caches the last stage and we use a multistage build. So I think we don't really get to take advantage of the caching here.

I'd like to propose (or at least explore and discuss) making our "official" ghcr.io devel images (the ones that get built on pushes to devel and feature branches) use a registry cache instead. Basically they would push a secondary cache image alongside the branch image.

So we'd have (for example): ghcr.io/ansible/awx_devel:devel and ghcr.io/ansible/awx_devel:devel_cache or something.

I can't guarantee this will speed things up, but I think there's a chance that it could.

Copy link
Member

Choose a reason for hiding this comment

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

At first glance, I don't think I disagree with your proposal for use of the registry cache, but we certainly want to keep that out of the current scope and I'd like @shanemcd to have a look. What we already have has been working relatively well to keep build times fast and there are sometimes slowdown you see due to out-of-date branches or dependency changes that don't apply to small patches and aren't a real problem.

@relrod
Copy link
Member Author

relrod commented Aug 30, 2023

@AlanCoding thanks for the review/comments.

I am planning to file a few issues for future improvements, the main one that comes to mind is nixing our github_ci_setup Makefile target (and related tools/docker-compose/ansible/smoke-test.yml).

These do basically the exact same thing as the new action. Let's standardize on one way to set up the dev env in CI.

@AlanCoding
Copy link
Member

When I introduced the github_ci_setup, it was because I lacked the immediate ability to do what you did with the uses: here. I get that you probably do want to build the image for these tests, and that will likely somehow intermingle the github actions YAML reuse. Honestly there's already existing stuff that hasn't been combined into the standard setup / running stuff.

@relrod
Copy link
Member Author

relrod commented Aug 31, 2023

@AlanCoding

I get that you probably do want to build the image for these tests, and that will likely somehow intermingle the github actions YAML reuse.

I don't think it would at all, we can just pass in another variable with with like I do for controlling if the UI should get built, and conditionalize based on that. If build-head, we make docker-compose-build, otherwise we pull the latest available image from ghcr.

Honestly there's already existing stuff that hasn't been combined into the standard setup / running stuff.

I wasn't sure what you meant by this.

@AlanCoding
Copy link
Member

I wasn't sure what you meant by this.

The github_ci_setup builds the image, but we have github workflow actions that run make docker-compose-build. To be DRY this shouldn't happen. That's what I mean. Because the github actions have expanded in scope so much, it would be good to aggressively consolidate it. It is large enough that it's a maintenance concern. What you're doing is going in the right direction.

If build-head, we make docker-compose-build, otherwise we pull the latest available image from ghcr.

Something's not clicking for me here, so I'm going to talk in fairly basic terms.

Some PRs will modify our dependencies and make corresponding API code changes (and errors will happen if image & code is out-of-sync) - this is basically guaranteed to happen. Some PRs will modify the collection.

It doesn't make sense to run unconditional tests without first building the image, because that would almost guarantee false-failures whenever something big happens. So running the tests unconditionally (which I'm not telling you to do) forces the issue of building the image.

I would be in favor of both making this conditional to awx_collection/ and making it non-voting in order to speed this up. In that context, it makes more sense to me to run without building the image. But in the long run, I don't see why we wouldn't build the image before running this.

@relrod relrod force-pushed the collection-integration branch 3 times, most recently from 3a18560 to 9598937 Compare September 3, 2023 07:23
.github/workflows/ci.yml Outdated Show resolved Hide resolved
There are a number of changes here:

- Abstract out a GHA composite action for running the dev environment
- Update the e2e tests to use that new abstracted action
- Introduce a new (matrixed) job for running collection integration
  tests. This splits the jobs up based on filename.
- Collect coverage info and generate an html report that people can
  download easily to see collection coverage info.
- Do some hacks to delete the intermediary coverage file artifacts
  which aren't needed after the job finishes.

Signed-off-by: Rick Elrod <[email protected]>
@relrod relrod merged commit d03a6a8 into ansible:devel Sep 5, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:awx_collection issues related to the collection for controlling AWX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants