k8snetworkplumbingwg / multi-networkpolicy-iptables

MultiNetworkPolicy iptable based implementation
Apache License 2.0
13 stars 19 forks source link

Enable end2end tests on pull requests #36

Closed zeeke closed 1 year ago

zeeke commented 1 year ago

It would be useful to see if a PR pass end2end tests before merging to main branch.

Signed-off-by: Andrea Panattoni apanatto@redhat.com

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3274924143


Totals Coverage Status
Change from base Build 3273651483: 0.0%
Covered Lines: 862
Relevant Lines: 1692

💛 - Coveralls
s1061123 commented 1 year ago

Without your PR change, e2e-kind tests are run as you see in current CI pipeline, https://github.com/k8snetworkplumbingwg/multi-networkpolicy-iptables/actions/runs/3266868905/jobs/5371235019 (in this PR)

So at least to me, I don't know what you want to do in the PR.

s1061123 commented 1 year ago

Understood the issue. So the root cause is this workflow (kind-e2e.yml) cannot be run due to the error, https://github.com/zeeke/multi-networkpolicy-iptables/actions/runs/3266782583/workflow

So once this issue is fixed, I believe the e2e tests will work fine in PR, without change of if: >.... Could you please change your PR to change - name: Test: simple -> - name: "Test: simple", only?

s1061123 commented 1 year ago

BTW, In future case, I recommend you to submit issue first, instead of PR.

zeeke commented 1 year ago

Understood the issue. So the root cause is this workflow (kind-e2e.yml) cannot be run due to the error, https://github.com/zeeke/multi-networkpolicy-iptables/actions/runs/3266782583/workflow

So once this issue is fixed, I believe the e2e tests will work fine in PR, without change of if: >.... Could you please change your PR to change - name: Test: simple -> - name: "Test: simple", only?

Other than the syntax error, the condition was preventing the tests to run in PRs. For example, in multi-networkpolicy-iptables/pull/35 there is no e2e-kind check in the checks list.

BTW, In future case, I recommend you to submit issue first, instead of PR.

It's much easier to talk over the code lines like you just did. It would be much harder to understand each other with text-only issues. Please, let's keep the process lean.

zeeke commented 1 year ago

Cool, I thought tests were not running for two reasons (yaml errors and if condition). But I was wrong: the only cause was yaml syntax error.

Indeed, I pushed the condition again and I can see the job e2e-kind / e2e-kind (pull_request) running.

BTW, I suggest putting a comment on that, because I still don't know what that condition is meant to achieve

s1061123 commented 1 year ago

Hi, thank you for the incorporate my comments. First.

It's much easier to talk over the code lines like you just did. It would be much harder to understand each other with text-only issues. Please, let's keep the process lean.

I am okey however, let me recommend again you to submit the issue first before PR. In this case, your first draft PR does not clearly capture the issue, I suppose. Submitting the issue in community, makes heads up to the community and we could discuss about whether the issue is real issue or not. In addition, we also discuss about how to fix that. It may seems you to redundant however, it may reduces the process by review comment. (If your fix design is not appropriate, reviewer may asks you to change the design, so both takes same time).

s1061123 commented 1 year ago

BTW, 'if' condition do nothing special. You could understanding by its document: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions