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

Rename codeql-analysis.yml to codeql.yml #3559

Closed

Conversation

naidneelttil
Copy link
Contributor

recently it seems that the default codeQL configuration name has changed to codeql.yml. this change would ensure that if anyone were to fork the project they don't have to change the filename to get coding alert issues.

recently it seems that the default codeQL configuration name has changed to codeql.yml. this change would ensure that if anyone were to fork the project they don't have to change the filename to get coding alert issues.
@github-actions github-actions bot added the CI Continuous integration label Apr 4, 2024
@echoix
Copy link
Member

echoix commented Apr 4, 2024

https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#:~:text=for%20example%2C%20by%20default%2C%20the%20workflow%20file%20for%20codeql%20code%20scanning%20is%20called%20codeql%2Danalysis.yml "For example, by default, the workflow file for CodeQL code scanning is called codeql-analysis.yml"

I don't seem to find that.

Do you have more info that I can find more about the change?

How I understand CodeQL works, it shouldn't be a problem to keep the old name. It fact it may be less of a problem.
For example, in January, in #3361 (merged Jan 15) when I recreated the CodeQL workflow, I went and used the language's canonical name "c-cpp" (not the alias "cpp"). Later on, I realized that there were some warnings in the job summaries (or a gray box instead of a check for the PR checks) for every PR run that mentioned that a configurations were missing. Look at #3431, merged in February. https://github.com/OSGeo/grass/runs/21664470025
image
image
The missing configurations on main were unexpected, since c-cpp and python worked in main. The twist was that the full length configuration tag/label was slightly different by default. So that meant there were 4 configurations. Issues that were fixed and merged weren't being closed in the list of issues.

I searched a bit, and to finally solve it (tried it in my fork before), I deleted the old configurations, and it cleaned those. On a next run on main, the correct issues were reopened if still present. That was in March, probably March 3.

Like this:
image

So, all this to show, that changing the file name will most probably change that configuration tag again. So that would have an impact again on everyone that already has a fork of grass and that enabled workflows (and thus might be running CodeQL when code scanning after forking).

So, what usability issue does it solve for new users?

(I'm not against if justified enough, that's why I took the time to show my experiences)

@wenzeslaus
Copy link
Member

Seems like the change is not necessary and may cause additional work. Closing. Thanks @naidneelttil anyway!

@wenzeslaus wenzeslaus closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants