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

Extra options #27

Merged
merged 9 commits into from
Apr 18, 2023
Merged

Extra options #27

merged 9 commits into from
Apr 18, 2023

Conversation

albthali
Copy link
Contributor

@albthali albthali commented Apr 4, 2023

Allow the extra options that aren't defined to be passed to Sonar Scanner. Any options that is not predefined can be passed via extra option in the project json or via environment variable. Environment variables has to be prefixed by SONAR and will be casted to lower case and any underscore will be replaced to a period. The order of importance is Environment variable over options over extra options. Environment > Options > Options.extra

@nx-cloud
Copy link

nx-cloud bot commented Apr 4, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 131fd9d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


🟥 Failed Commands
nx affected --target=e2e --parallel=1
✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

Copy link
Owner

Choose a reason for hiding this comment

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

revert changes

@@ -11,4 +11,6 @@ export interface ScanExecutorSchema {
skipImplicitDeps?: boolean;
testInclusions?: string;
verbose?: boolean;
excludedOptions?: string[];
Copy link
Owner

Choose a reason for hiding this comment

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

why is excludedOptions needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an artifact from an early implementation. It is no longer needed. It will be removed

serverUrl: options.hostUrl,
options: scannerOptions,
});
return {
success: success,
scannerOptions: scannerOptions,
Copy link
Owner

Choose a reason for hiding this comment

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

executors should only return {success: success}

Copy link
Contributor Author

@albthali albthali Apr 11, 2023

Choose a reason for hiding this comment

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

This is the function that that does the scanning. The executor function is unchanged and returns {success: success}. The reasoning behind this change is so that I can see if the scannerOptions respects the extraOptions and environment variables set by the user.

Another way to check if the extra options and environment variables are respected without changing the function signature is to move it a different function.

function getScannerOptions(options:ScanExecutorSchema,sources:string, lcovPaths:string, branch: string):  { [option: string]: string } {
 let scannerOptions: { [option: string]: string } = {
   'sonar.exclusions': options.exclusions,
   'sonar.javascript.lcov.reportPaths': lcovPaths,
   'sonar.language': 'ts',
   'sonar.login': process.env.SONAR_LOGIN,
   'sonar.organization': options.organization,
   'sonar.password': process.env.SONAR_PASSWORD,
   'sonar.projectKey': options.projectKey,
   'sonar.projectName': options.projectName,
   'sonar.projectVersion': options.projectVersion,
   'sonar.qualitygate.timeout': options.qualityGateTimeout,
   'sonar.qualitygate.wait': String(options.qualityGate),
   'sonar.scm.provider': 'git',
   'sonar.sources': sources,
   'sonar.sourceEncoding': 'UTF-8',
   'sonar.tests': sources,
   'sonar.test.inclusions': options.testInclusions,
   'sonar.typescript.tsconfigPath': 'tsconfig.base.json',
   'sonar.verbose': String(options.verbose),
 };
 if (options.branches) {
   scannerOptions["sonar.branch.name"] = branch
 }
 scannerOptions = combineOptions(
   new ExtraMarshaller(options.extra),
   new EnvMarshaller(),
   scannerOptions
 )
 return scannerOptions
}

I prefer to move it to a different function as it would ease testing but I did not want to change the code too much.

@albthali
Copy link
Contributor Author

Any idea why the pipeline is failing? It looks like there are missing credentials, i.e SONAR_TOKEN. I tested it locally to check if it is working and it seems to be working fine. Keep in mind that I had to modify the following in the e2e test to make it work

  const hostUrl = process.env.SONAR_HOST_URL || 'https://sonarcloud.io';
  const projectKey = process.env.SONAR_PROJECT_KEY || 'nx-sonarqube-e2e';
  const organization = process.env.SONAR_ORGANIZATION || 'koliveira15';

@koliveira15
Copy link
Owner

Any idea why the pipeline is failing? It looks like there are missing credentials, i.e SONAR_TOKEN. I tested it locally to check if it is working and it seems to be working fine. Keep in mind that I had to modify the following in the e2e test to make it work

  const hostUrl = process.env.SONAR_HOST_URL || 'https://sonarcloud.io';
  const projectKey = process.env.SONAR_PROJECT_KEY || 'nx-sonarqube-e2e';
  const organization = process.env.SONAR_ORGANIZATION || 'koliveira15';

@albthali I will look into e2e, you can ignore for now. Resolve the merge conflict and you should be okay.

@koliveira15 koliveira15 merged commit fa3326e into koliveira15:main Apr 18, 2023
4 of 5 checks passed
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.

None yet

2 participants