-
Notifications
You must be signed in to change notification settings - Fork 376
Rust: remove shipped feature flag #2960
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK to merge if you are happy to break Rust analyses for those older versions of the CLI. See my thoughts in the comment I added. There's also one typo in the comment.
// this point, if they want rust support they should chose a version which supports it | ||
// publicly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might break Rust analysis for users who have pinned v3
(or similar) of the Action as well as a CodeQL CLI >= 2.19.3 && < 2.22.1
and who used CODEQL_ENABLE_EXPERIMENTAL_FEATURES
. I agree that it's not unreasonable to ask users to upgrade to a newer CLI version in that case, but perhaps we should give them some warning first before breaking their workflows over night when the next Action release happens? That said, it's not clear to me that a warning in the output would necessarily be noticed. I don't know if we have any data to know how many people would be affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that situation (version 2.19.3..2.22.1
and CODEQL_ENABLE_EXPERIMENTAL_FEATURES=true
) codeql resolve languages
does give rust, which means that (await codeql.resolveLanguages()).rust
is not undefined. The only change then is that users lose the Experimental Rust analysis enabled
log line, but no functional change happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uh, actually, now I see that that log line was dead code anyway!
Clarify comments regarding rust support in codeql versions
Merge / deployment checklist