Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(configdocsgenerator): use Spring to parse Yaml into InspectitConfig #1362

Merged

Conversation

aaronweissler
Copy link
Member

@aaronweissler aaronweissler commented Mar 24, 2022

This change is Reviewable

@aaronweissler aaronweissler force-pushed the feature/docs-generator-spring branch from bbcf80e to 4a7635f Compare March 24, 2022 09:31
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #1362 (b39b1f5) into master (d37a7dc) will decrease coverage by 1.86%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1362      +/-   ##
============================================
- Coverage     83.00%   81.14%   -1.86%     
- Complexity     1736     2070     +334     
============================================
  Files           174      210      +36     
  Lines          5366     6591    +1225     
  Branches        650      784     +134     
============================================
+ Hits           4454     5348     +894     
- Misses          675      945     +270     
- Partials        237      298      +61     
Impacted Files Coverage Δ
...spectit/ocelot/core/utils/WeakMethodReference.java 0.00% <0.00%> (-77.27%) ⬇️
...strap/correlation/noop/NoopLogTraceCorrelator.java 28.57% <0.00%> (-42.86%) ⬇️
...celot/core/exporter/PrometheusExporterService.java 81.48% <0.00%> (-13.97%) ⬇️
...ial/ScheduledExecutorContextPropagationSensor.java 82.05% <0.00%> (-12.39%) ⬇️
...pectit/ocelot/config/loaders/ConfigFileLoader.java 82.61% <0.00%> (-8.70%) ⬇️
...lot/bootstrap/context/noop/NoopContextManager.java 10.00% <0.00%> (-6.67%) ⬇️
...core/instrumentation/InstrumentationTriggerer.java 85.19% <0.00%> (-5.63%) ⬇️
...rocks/inspectit/ocelot/config/utils/CaseUtils.java 90.48% <0.00%> (-4.98%) ⬇️
...t/ocelot/core/instrumentation/hook/MethodHook.java 95.35% <0.00%> (-4.65%) ⬇️
...del/metrics/definition/ViewDefinitionSettings.java 45.45% <0.00%> (-4.55%) ⬇️
... and 111 more

@mariusoe mariusoe changed the title fix/feature(configdocsgenerator) - Use Spring to parse Yaml into InspectitConfig fix(configdocsgenerator): use Spring to parse Yaml into InspectitConfig Mar 25, 2022
@mariusoe mariusoe added bug Something isn't working area/config-server labels Mar 25, 2022
Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronweissler)


components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/parsing/ConfigParser.java, line 42 at r2 (raw file):

        if (!StringUtils.isEmpty(configYaml)) {

            File tempFile = File.createTempFile("temp-", ".tmp");

The file is then named temp-{...}.tmp?
Maybe sth like inspectit-config-{...} is more appropriate?


components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/parsing/ConfigParser.java, line 49 at r2 (raw file):

            // Read yaml into Properties
            YamlPropertiesFactoryBean factory = new YamlPropertiesFactoryBean();
            factory.setResources(new FileUrlResource(tempFile.getAbsolutePath()));

What is the difference between these lines of code and StringUtils.readYamlFiles(new FileUrlResource(tempFile.getAbsolutePath()));?


components/inspectit-ocelot-configdocsgenerator/src/main/java/inspectit/ocelot/configdocsgenerator/parsing/ConfigParser.java, line 51 at r2 (raw file):

            factory.setResources(new FileUrlResource(tempFile.getAbsolutePath()));
            factory.setDocumentMatchers((profile) -> YamlProcessor.MatchStatus.FOUND);
            factory.afterPropertiesSet();

I think this line has no effect if you are not calling factory.setSingleton() before.
And I think this is also redundant if you call factory.getObject() immediately afterwards.

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aaronweissler)

@heiko-holz heiko-holz merged commit b93668a into inspectIT:master Mar 29, 2022
@aaronweissler aaronweissler deleted the feature/docs-generator-spring branch June 7, 2022 12:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area/config-server bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants