add if statement for pr & push event

[alonyb]

This should fix #462 and #475 (force-pushed cases) logs

[ahmetb]

Thanks. I am currently -1 for this duplication.
I am ~80% sure there’s a better way to do this, if not, I am questioning capabilities of Github Workflows system. Do you mind asking on their community forums if there’s a way to do this cleanly?

[ahmetb]

We need plugin validation to run only in PRs in all honesty.

Once merged, it’s too late to check manifests as we can’t act on it. Am i right @corneliusweig ?

[alonyb]

Thanks. I am currently -1 for this duplication.
I am ~80% sure there’s a better way to do this, if not, I am questioning capabilities of Github Workflows system. Do you mind asking on their community forums if there’s a way to do this cleanly?

Sure, that was a quick solution. For those cases. But there are better options.

[corneliusweig]

I agree. The tests should only run on PR and pushes to pull requests.

The duplication between the steps does not make sense to me. I still don't understand why we don't just do

git diff --name-only --diff-filter=AM origin/master ${{ github.sha }} -- plugins/

This would work fine with force push AND it would always run the checks for everything that is about to be merged into master. Doesn't that sound right?

[ahmetb]

Yeah. And travis was gracious enough to provide the right value as env vars.

[alonyb]

I agree. The tests should only run on PR and pushes to pull requests.

The duplication between the steps does not make sense to me. I still don't understand why we don't just do

git diff --name-only --diff-filter=AM origin/master ${{ github.sha }} -- plugins/

This would work fine with force push AND it would always run the checks for everything that is about to be merged into master. Doesn't that sound right?

In PR event it is the better option. But for push event, I needed a hash to compare. Now that it's just PR it's simple.

[ahmetb]

@alonyb can you update with the suggested changes? I think we don't need to build on "push" (I'm assuming it means pushing to master). We still need to rebuild on pushes to PR branches.

[alonyb]

can you update with the suggested changes? I think we don't need to build on "push" (I'm assuming it means pushing to master). We still need to rebuild on pushes to PR branches.

I just deleted the push event, however, I could only be able to push on the master branch. Or only in PR, depending on what is needed now.

[ahmetb]

Thanks! Let's give this a shot.
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, alonyb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ahmetb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[corneliusweig]

@alonyb Looks like we have a problem, see #485

[corneliusweig]

PR #486 should fix this.