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

actions: Update codeql triggers #16251

Closed
wants to merge 1 commit into from
Closed

actions: Update codeql triggers #16251

wants to merge 1 commit into from

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented May 20, 2021

Trigger codeql on:

  • push to master or v*.* branch
  • pull request against any branch

Signed-off-by: Michi Mutsuzaki michi@isovalent.com

branches:
- master
schedule:
- cron: "45 6 * * 3"
Copy link
Member

Choose a reason for hiding this comment

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

#16241 removed the master run instead of the scheduled run. I'd strongly prefer to keep the scheduled run. It's less frequent and we don't need to run for every single push to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good i'll put it back. how about pull requests against release branches?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's fine and it's already included in pull_request: {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 sounds good just wanted to confirm. before this pr we were only running against master branch PRs i think.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this setup will work? See current warnings on CodeQL from this PR's Actions: https://github.com/cilium/cilium/actions/runs/861152333
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added back push to master trigger 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Warning is still here. I don't get it.
https://github.com/cilium/cilium/actions/runs/867731119

Copy link
Member

Choose a reason for hiding this comment

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

I think that's just because the workflow on master, doesn't run on master:

push:
branches:
- v1.10
- v1.9
- v1.8

It should be fixed once this is merged. We should really find another way to limit CodeQL runs though, because they take a while :(

Copy link
Member

Choose a reason for hiding this comment

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

I'll put my stamp on the current version but agree that it is a poor stopgap.

@aanm aanm assigned aanm and unassigned aanm May 21, 2021
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

See question above.

Trigger codeql on:

- push to master or v*.* branch
- pull request against any branch

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent
Copy link
Contributor Author

test-me-please

branches:
- master
- v*.*
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I follow this. This will mean that any branch starting with v would be triggered by this, it's not always the case. There are branches starting with v that do not belong to stable branches.

@pchaigno
Copy link
Member

pchaigno commented Jun 3, 2021

Is this still needed now that #16389 is merged?

@michi-covalent
Copy link
Contributor Author

Is this still needed now that #16389 is merged?

not needed after #16389 you all are moving so fast i can't keep up 🤯

@michi-covalent michi-covalent deleted the pr/michi/codeql branch June 3, 2021 21:13
@pchaigno
Copy link
Member

pchaigno commented Jun 3, 2021

Sorry, I didn't realize this had not been merged yet when I opened #16389. I probably understood the issue better thanks to the discussion here.

@michi-covalent
Copy link
Contributor Author

@pchaigno btw my previous test-me-please was just to trigger the tests after i rebased this pull request, not to retry because of flakes. i promise i'm not blindly restarting tests anymore 😅

@pchaigno
Copy link
Member

pchaigno commented Jun 3, 2021

Héhé, my smiley was because you're triggering Jenkins tests on a PR that doesn't changes anything covered by the Jenkins tests 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants