Skip to content

Conversation

@rohithkrajan
Copy link

@rohithkrajan rohithkrajan commented Apr 17, 2020

implemented a custom rule to check if a component used is correctly named and it exists in the path.

The rule will apply if any of following component usage is found


component implements = 'api.interface.ComponentName'
component extends = "components.xmlrpc.Service"
var defaults = new DefaultValues(); or configuration = new components.configuration.Manager('VoucherEngine')
`property name = 'reporter' inject = 'components.error.errorfunctions';
createObject('component', 'components.voucher.Driver')
exporter = server.di.getInstance('components.configuration.Manager');

The rule checks two things based on the configuration (.cflintrc)


{
        "code": "COMPONENT_NOT_FOUND"
    },
    {
        "code": "INVALID_COMPONENT_USAGE"
    }

image

It can also throw error if component does exist but the name used is not pascalcase.

image

we can avoid the invalid component usage by removing INVALID_COMPONENT_USAGE from cflintrc.

This PR contains some kind of integration tests and unit tests for possible combinations of component usage and all the tests

image

@rohithkrajan rohithkrajan requested review from cosminj and jbutkus April 21, 2020 08:45
Copy link

@cosminj cosminj left a comment

Choose a reason for hiding this comment

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

It is quite a large PR, and I've only gone through the first file.

//work around to find web root
// initialize known sub folders of web root
//without this, to find the web root , we have to search for server/web.xml coldfusion configuration file
String[] knownRootSubDirs = {"api", "voucherengine", "components", "avisfuelcard", "commandchain", "ioc", "parrot", "paycard", "santam", "voucherpos", "access", "customTags", "common"};
Copy link

Choose a reason for hiding this comment

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

These should be configurable in an external given file or environment variable.

Copy link
Author

@rohithkrajan rohithkrajan Jun 26, 2020

Choose a reason for hiding this comment

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

👍 made a change where we can pass these values using properties . We have a default set of values now and can override the default set using java -Dknownsubdirs=api,voucherengine,components ...

@modernuniverse
Copy link

Let's hold on these changes. Since cflint story is back to SRE backlog, I would like to revisit the story and see what makes sense most sense to do from the SRE team's perspective.

Copy link

@jbutkus jbutkus left a comment

Choose a reason for hiding this comment

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

@tonym128 shall this be handed over to your team, as part of the wider effort?

@modernuniverse
Copy link

In my direct conversation with Rohith, we agreed he was going to merge this and provide me a new jar file with these changes included. My impression was that there was not much work left on this. @rohith could you confirm?

@rohithkrajan
Copy link
Author

In my direct conversation with Rohith, we agreed he was going to merge this and provide me a new jar file with these changes included. My impression was that there was not much work left on this. @rohith could you confirm?

yes @modernuniverse . i have pushed an update for the review comments and we should be good to merge after this.

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.

5 participants