community.grafana icon indicating copy to clipboard operation
community.grafana copied to clipboard

Replace GHA boilerplate with the use of an Action

Open webknjaz opened this issue 3 years ago • 8 comments

This patch replaces manually maintained copy-paste of ansible-test invocations with the use of a reusable GitHub Action1 that wraps it allowing to only keep the matrix on the project side allowing the reusable bits to be maintained separately and be shared across the community.

This has been earlier implemented for the community.digitalocean collection2 among others.

SUMMARY

See above.

ISSUE TYPE
  • Maintenance Pull Request
  • Testing Pull Request
COMPONENT NAME

.github/workflows/ansible-test.yml

ADDITIONAL INFORMATION

This is also on the way of being included into the default collection skeleton at https://github.com/ansible/ansible/pull/74901/files#diff-096f7bb125bbac5751a11cfa3f7682b8d7c7c8cfffb0feddd10fb78e7346950d.

webknjaz avatar Jul 22 '22 16:07 webknjaz

@webknjaz based on latest news I guess we have to update the matrix to have tests on devel no more using Python 3.8

Am I right ?

rrey avatar Jul 24 '22 07:07 rrey

Am I right ?

Fair enough.

webknjaz avatar Jul 25 '22 15:07 webknjaz

Codecov Report

Merging #266 (2e5fda7) into main (33fa3de) will decrease coverage by 25.45%. The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #266       +/-   ##
===========================================
- Coverage   69.45%   44.00%   -25.46%     
===========================================
  Files          16       16               
  Lines        1709     1709               
  Branches      320      320               
===========================================
- Hits         1187      752      -435     
- Misses        385      937      +552     
+ Partials      137       20      -117     
Flag Coverage Δ
integration ?
sanity 22.81% <ø> (ø)
units 73.54% <ø> (+0.13%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
plugins/modules/grafana_datasource.py 14.60% <0.00%> (-60.68%) :arrow_down:
plugins/modules/grafana_organization.py 33.80% <0.00%> (-50.71%) :arrow_down:
plugins/modules/grafana_folder.py 28.28% <0.00%> (-47.48%) :arrow_down:
plugins/modules/grafana_user.py 51.61% <0.00%> (-39.79%) :arrow_down:
plugins/modules/grafana_notification_channel.py 18.04% <0.00%> (-39.52%) :arrow_down:
plugins/modules/grafana_dashboard.py 14.11% <0.00%> (-37.65%) :arrow_down:
plugins/modules/grafana_team.py 65.06% <0.00%> (-21.24%) :arrow_down:
.../modules/grafana/grafana_team/test_grafana_team.py 97.42% <0.00%> (+0.42%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 33fa3de...2e5fda7. Read the comment docs.

codecov[bot] avatar Jul 25 '22 15:07 codecov[bot]

I'm happy with this. Maintainers welcome to merge once they are happy.

It's not ready yet. I've faced issues with integration tests.

@rrey I've found that parametrization of the grafana is a bit weird. Have you thought of remaking it somehow to make it flexible? FWIW I was thinking about reverting the integration test changes for now, merging only units+sanity and trying to solve the integration problem in follow-ups. WDYT?

webknjaz avatar Jul 25 '22 22:07 webknjaz

@rrey I've found that parametrization of the grafana is a bit weird. Have you thought of remaking it somehow to make it flexible?

@webknjaz Can you elaborate ? Maybe it is the use of Github Actions services that bothers you ?

If yes, It is actually super useful because it is a simple way to have Grafana running and test against it. It allows to avoid anything related to Grafana installation. I had a problem with ansible-test when I setup the CI early in the Collection creation because this mechanism is not compatible with ansible-test running in docker (the github service and the ansible-test container won't be sharing network).

That is why the integration tests are not using the --docker option for integration tests.

FWIW I was thinking about reverting the integration test changes for now, merging only units+sanity and trying to solve the integration problem in follow-ups. WDYT?

Sounds good !

rrey avatar Aug 15 '22 17:08 rrey

@webknjaz Can you elaborate ? Maybe it is the use of Github Actions services that bothers you ?

If yes, It is actually super useful because it is a simple way to have Grafana running and test against it. It allows to avoid anything related to Grafana installation. I had a problem with ansible-test when I setup the CI early in the Collection creation because this mechanism is not compatible with ansible-test running in docker (the github service and the ansible-test container won't be sharing network).

That is why the integration tests are not using the --docker option for integration tests.

So basically, the containers that ansible-test spawns end up being on a different docker network than the ones that the GitHub Actions Runner starts. It's visible at https://github.com/ansible-collections/community.grafana/runs/7510549704?check_suite_focus=true#step:2:14.

And because of that, the tests can't communicate with the Grafana instance. I was considering improving the action to auto-detect that network (it's name is not static across runs) and adding a --docker-network param to the invocation. Another solution (likely better) is integrating the Grafana service invocation into the tests. This would allow running the tests locally the same way. Although, I'm not sure how to organize this best — ansible-test in ansible-core has a number of built-in "cloud service providers/stubs" for testing the Core itself but I don't think that we have any way for the collections to implement their own "providers" for integration testing. Also, at the moment, ansible-test doesn't have a parametrization mechanism. Maybe, it could be implemented through run.sh reading a grafana version value from a file on disk and GHA modifying that file prior to calling the action.

But for now, go ahead and merge #270. I'll keep thinking of how to properly instrument this on the action's side. No timeline promises, though.

webknjaz avatar Aug 19 '22 17:08 webknjaz

I was considering improving the action to auto-detect that network (it's name is not static across runs)

https://docs.github.com/en/actions/learn-github-actions/contexts#job-context documents that there's ${{ job.services.<service_id>.network }} so maybe autodetection isn't really needed.

webknjaz avatar Aug 20 '22 17:08 webknjaz

Cool ! Happy to test anything here if you add a feature in ansible-test

rrey avatar Aug 20 '22 18:08 rrey

New attempt: https://github.com/ansible-collections/community.grafana/pull/276.

webknjaz avatar Sep 23 '22 13:09 webknjaz

v1.9.0 implemented auto-exposing the container network that GH creates in workflows: https://github.com/ansible-community/ansible-test-gh-action/discussions/37.

webknjaz avatar Sep 23 '22 13:09 webknjaz