search

ansible-collections / overview

Collections overview, how to request a namespace
GNU General Public License v3.0
247 stars 49 forks source link

Change requirements related to running tests against release commit #206

Closed Andersson007 closed 2 years ago

Andersson007 commented 2 years ago
SUMMARY

Relates to https://github.com/ansible-collections/ansible-inclusion/discussions/44#discussioncomment-2889787. As integration/unit tests are optional anyway and it can be expensive to run them against release commits, i suggest making their run optional.

Andersson007 commented 2 years ago

any thoughts folks? If there are no objections, i'll create a community topic to vote on this to get merged

mariolenz commented 2 years ago

I'm not really sure about this. On the one hand, you want to make integration/unit tests optional for release commits because they can be expensive. On the other hand, I see the requirement to run all CI tests against every pull request and to run all CI tests regularly.

I agree that integration/unit tests can be expensive. But if the requierment is to run those (expansive) tests both against every pull request and regulary, I don't see that we save much.

Don't get me wrong, I'm not against relaxing the test requirements. But shouldn't a release commit be better tested than a "normal" PR?

Andersson007 commented 2 years ago

I'm not really sure about this. On the one hand, you want to make integration/unit tests optional for release commits because they can be expensive. On the other hand, I see the requirement to run all CI tests against every pull request and to run all CI tests regularly.

I agree that integration/unit tests can be expensive. But if the requierment is to run those (expansive) tests both against every pull request and regulary, I don't see that we save much.

Don't get me wrong, I'm not against relaxing the test requirements. But shouldn't a release commit be better tested than a "normal" PR?

@mariolenz , all sounds sensible to me. On the other hand, integration and unit tests are optional for the collection anyway. imo it feels a bit strange if we, as now,

This can lead to situations, like in @resmo 's case, when the requirements will force him to remove the integration and unit tests to formally satisfy the requirements. I would recommend running all the tests but would keep only sanity run mandatory as only their presence in collections is mandatory.

UPDATE: also i don't see a requirement for collections that the publishing process must be automated, so maintainers can publish the collections even manually if they like.

felixfontein commented 2 years ago

How about not dropping the requirement which tests must be run for a release commit, but instead dropping the requirement that all of them must pass, and specify exactly which are allowed to pass under which circumstances?

Andersson007 commented 2 years ago

How about not dropping the requirement which tests must be run for a release commit, but instead dropping the requirement that all of them must pass, and specify exactly which are allowed to pass under which circumstances?

Feel free to suggest the wording, i'm not sure i understand. I'd better drop the requirement because of the reasons explained in https://github.com/ansible-collections/overview/pull/206#issuecomment-1149594097

felixfontein commented 2 years ago

How about not dropping the requirement which tests must be run for a release commit, but instead dropping the requirement that all of them must pass, and specify exactly which are allowed to pass under which circumstances?

After discussing this at Wednesday's meeting, I now agree that requiring sanity checks is enough. In case unit and/or integration tests run, I still recommend that in case they do not pass, they should be looked at to investigate whether they block the release or not. This leaves open the door for failures to be ignored (even automatically, because it says 'should'), but right now this has not been resulting in a decreased quality of collection releases. (Skipping sanity tests did though, but that wasn't allowed so far either, and won't be allowed in the future.)

Andersson007 commented 2 years ago

thanks everyone!