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

Support development dependencies for the Gradle detector #878

Merged
merged 11 commits into from
Feb 27, 2024

Conversation

joakley-msft
Copy link
Member

Lack of development dependency detection for Gradle is a problem for Android teams, especially in the context of Component Governance alerts. Unfortunately Gradle doesn't provide enough information to definitively identify dev dependencies in all cases, so manual configuration is required. This change adds dev dependency classification through two mechanisms

  1. buildscript-gradle.lockfile and settings-gradle.lockfile contain only build-system dependencies, so always classify these as development dependencies.
  2. Processing based on two new environment variables: GRADLE_PROD_CONFIGURATIONS_REGEX and GRADLE_DEV_CONFIGURATIONS_REGEX. Gradle lockfiles indicate which Gradle configuration(s) each dependency is required by. GRADLE_PROD_CONFIGURATIONS_REGEX allows specifying production configurations explicitly. All other configurations are considered development. Alternately, dev configurations may be specified in GRADLE_DEV_CONFIGURATIONS_REGEX and all others are considered production.

Lack of development dependency detection for Gradle is a problem for
Android teams, especially in the context of Component Governance
alerts. Unfortunately Gradle doesn't provide enough information to
definitively identify dev dependencies in all cases, so manual
configuration is required. This change adds dev dependency
classification through two mechanisms

1. `buildscript-gradle.lockfile` and `settings-gradle.lockfile`
   contain only build-system dependencies, so always classify these as
   development dependencies.
2. Processing based on two new environment variables:
   `GRADLE_PROD_CONFIGURATIONS_REGEX` and
   `GRADLE_DEV_CONFIGURATIONS_REGEX`. Gradle lockfiles indicate which
   Gradle configuration(s) each dependency is required by.
   `GRADLE_PROD_CONFIGURATIONS_REGEX` allows specifying
   production configurations explicitly. All other configurations are
   considered development. Alternately, dev configurations may be
   specified in `GRADLE_DEV_CONFIGURATIONS_REGEX` and all others are
   considered production.
@joakley-msft joakley-msft requested a review from a team as a code owner October 26, 2023 17:03
@rygo-msft
Copy link

CG is auto-injected into all production pipelines, so I fear that maintaining an accurate configuration list in an environment variable will be near-impossible. CG operates on lockfiles as checked-in files, so I think ultimately the config needs to be a checked-in file as well.

@joakley-msft
Copy link
Member Author

CG is auto-injected into all production pipelines, so I fear that maintaining an accurate configuration list in an environment variable will be near-impossible.

I'm aware, but not sure why we couldn't set the variable in all production pipelines too?

CG operates on lockfiles as checked-in files, so I think ultimately the config needs to be a checked-in file as well.

I'd like input from the CG/component-detection owners before proceeding further here. There are existing configurations for specific detectors with environment variables, and I don't see any precedent for file-based configuration in this tool. I'm happy to go either direction based on guidance/preference of the owners here.

@joakley-msft
Copy link
Member Author

Thanks for the review @melotic. Before I make changes to address your comments on the environment variable access and naming, could you or @JamieMagee weigh in on Ryan's suggestion to use a configuration file instead of environment variables? I'm happy to go with either approach. I agree with @rygo-msft 's point on the benefit of a configuration file, but am not sure if it would be too inconsistent with the rest of the component-detection tool.

@joakley-msft
Copy link
Member Author

Hi @melotic, @JamieMagee -- any feedback on the configuration approach discussion (environment vs file)?

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c2cfd4d) 75.3% compared to head (a412d3e) 75.4%.

❗ Current head a412d3e differs from pull request most recent head 67f5998. Consider uploading reports for the commit 67f5998 to get more accurate results

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #878   +/-   ##
=====================================
  Coverage   75.3%   75.4%           
=====================================
  Files        236     236           
  Lines      10353   10386   +33     
  Branches    1025    1032    +7     
=====================================
+ Hits        7800    7833   +33     
  Misses      2269    2269           
  Partials     284     284           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rygo-msft
Copy link

I'm still unclear how this list of developer configurations will be built and maintained in practice. A typical android project will have 20+ different configurations and that number grows even large when build flavors are used.

Because lockfiles are static files, they will be scanned even for pipelines unrelated to the gradle build. These config values will need to be copied and initialized in every pipeline somehow. This is particularly difficult to achieve as static analysis jobs are typically injected without pipeline author input. There are cases where pipeline authors cannot provide configuration these injected jobs.

@joakley-msft
Copy link
Member Author

I would still like to hear from the repo maintainers if they would accept a change that read from a configuration file instead of environment variables. I agree that would be somewhat preferable in this case.

Ryan, can you give an example of a case where the pipeline authors cannot provide configuration via environment variables though? I was assuming we would just run the task setting up the environment variables before the component detection task.

@rygo-msft
Copy link

Governed Templates inject static analysis jobs that run standardized task sets and do not take user provided tasks.
Even in the case where this is an option, there are often many pipelines running from the same repo. Most of those will not run a gradle build at all, but need to be able to configure CG correctly because the analysis is on the static files.

@joakley-msft
Copy link
Member Author

We don't need to run a gradle build, just to be able to set up environment variables. But I wasn't familiar with the auto-injected static analysis jobs, I agree that's a concern and a good motivation for allowing configuration by file instead of environment variable. I'll pause here to see if Anna, Jamie or others can weigh in.

@cobya
Copy link
Contributor

cobya commented Feb 14, 2024

I would prefer a configuration file based approach in the long term, but I think it's acceptable while we iterate on the idea of how this kind of configuration should be maintained. In the future I'd like to see a detection-config.json file or similar with a given schema that can be passed to all of the detectors which accept additional environment variables.

In the meantime, let me go ahead and review the rest.

@joakley-msft joakley-msft requested a review from cobya February 19, 2024 14:46
@cobya cobya merged commit f85b6c4 into microsoft:main Feb 27, 2024
18 of 21 checks passed
Copy link

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

  • The detector detects more or fewer components than before
  • The detector generates different parent/child graph relationships than before
  • The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

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.

4 participants