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

Add logging statements declaring state of the cli_config_file_enabled #1281

Merged
merged 3 commits into from
Oct 4, 2022

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Sep 30, 2022

Minor change to insert a log statement to keep track of the new feature flag. It's possible to determine this otherwise, but this makes it easier to spot.

Also, Avoid using single value as array

The user config parser in the CLI doesn't yet support it.

@aeisenberg aeisenberg requested a review from a team as a code owner September 30, 2022 23:07
src/analyze.ts Outdated

if (await util.useCodeScanningConfigInCli(codeql, featureFlags)) {
logger.info(
"Code Scanning configuration file being processed in the codeql-action."
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong way round!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arrrgh!

src/config-utils.ts Outdated Show resolved Hide resolved
It's possible to determine this otherwise, but this makes it easier to
spot.
@aeisenberg aeisenberg force-pushed the aeisenberg/cli-config-processing branch from f8605b9 to 6ace05b Compare October 1, 2022 19:03
@aeisenberg
Copy link
Contributor Author

aeisenberg commented Oct 1, 2022

Oh...need to change how the query filters tests work. Some of them are explicitly expecting the config to be processed by the action and others the CLI. I will need to change how the env var is working for them.

Edit: Hmmm...the error message is a little more worrying. Maybe there's something really wrong here.

@aeisenberg aeisenberg marked this pull request as draft October 2, 2022 21:16
The user config parser in the CLI doesn't yet support it.
@aeisenberg aeisenberg force-pushed the aeisenberg/cli-config-processing branch from 1861036 to 59fbe34 Compare October 3, 2022 00:13
@aeisenberg aeisenberg marked this pull request as ready for review October 3, 2022 00:48
adityasharad
adityasharad previously approved these changes Oct 3, 2022
src/util.ts Outdated
@@ -817,6 +817,22 @@ export async function useCodeScanningConfigInCli(
return await codeQlVersionAbove(codeql, CODEQL_VERSION_CONFIG_FILES);
}

export async function logCodeScanningConfgInCli(
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo in name.

Comment on lines +1708 to 1710
await logCodeScanningConfigInCli(codeQL, featureFlags, logger);

if (!(await useCodeScanningConfigInCli(codeQL, featureFlags))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will hit the feature flags endpoint twice right? (And I think we don't attempt to add cache parameters to the API requests.) Can we make it call only once? Not sure of the cleanest way; perhaps the logCodeScanningConfigInCli function should just accept the boolean result of the use function, without calling use.

Copy link
Contributor Author

@aeisenberg aeisenberg Oct 4, 2022

Choose a reason for hiding this comment

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

Hmmm...you're right. Feature flags values are not being cached. Rather than implementing a workaround for useCodeScanningConfigInCli only, I think it makes more sense to simply add the caching (it's not too much code).

What do you think about my doing this in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh fancy! Nothing to worry about then.

@aeisenberg aeisenberg merged commit f359ba7 into main Oct 4, 2022
@aeisenberg aeisenberg deleted the aeisenberg/cli-config-processing branch October 4, 2022 19:30
@github-actions github-actions bot mentioned this pull request Oct 6, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants