search

konveyor / ci

Apache License 2.0
0 stars 12 forks source link

Stabilize rulesets changes #27

Closed aufi closed 4 months ago

aufi commented 9 months ago

There is a https://github.com/konveyor/rulesets repository that contains analysis rules converted from windup (by a windup-shim). These rules define issues created by the analysis process.

The latest update of ruleset https://github.com/konveyor/rulesets/pull/35 introduced few changes:

There were few quick fixes addressing issues above created on Friday Dec 8th. For a long-term stability, following steps should be done (feel free to modify or add new ones)

Proposal

jmle commented 9 months ago

I think we need to bare in mind a few things regarding the converted rules:

  1. We might still need to change the format of the rules in the future.
  2. There will probably be new windup rules that we need to convert.
  3. The shim is stuck at the moment at windup-rulesets version 6.2.3, but we manually added/altered some rules after that.

Given this, I think we need to:

  1. Use windup-rulesets as the single source of truth. No manual modifications should be done to our rulesets.
  2. Use the shim to make all the changes needed to the rules so that we don't need to modify them afterwards.
  3. Translate all windup rules, also the ones from 6.2.3 on. I think that version was set to achieve the goal of 80% coverage in the past, but we need to move forward and bring everything. ATM we have almost everything, but some of the stuff was added manually. This breaks conversion.
  4. Maybe have a document with information regarding decisions taken and conversion strategies. For instance, I might have accidentally added potential rules, maybe it was decided in the past that we didn't want to show them?

What do you think?

shawn-hurley commented 9 months ago

There will probably be new windup rules that we need to convert.

I disagree, all new rules that we are supporting once we release must be in the new format. Supporting two formats is already too much of a burden.

The shim is stuck at the moment at windup-rulesets version 6.2.3, but we manually added/altered some rules after that.

This is by design; we have to move to a world where we are not talking about windup rules, but we are talking about our rulesets. The shim and conversation were always meant to be a starting point and to move forward with our rulesets.

Is there some other reason?

jmle commented 9 months ago

I disagree, all new rules that we are supporting once we release must be in the new format. Supporting two formats is already too much of a burden.

I'm not proposing to support two different formats. There may be new rules coming from windup in the future that we will want to move over to konveyor. Of course they will be in the new format once they are translated.

This is by design; we have to move to a world where we are not talking about windup rules, but we are talking about our rulesets. The shim and conversation were always meant to be a starting point and to move forward with our rulesets.

I agree with this, but again, I think we will probably have to move new rules over to konveyor. There will be a transitional period in which both tools will be available and new rules will be written in windup, and I think it's easier to translate them automatically rather than rewrite them by hand.

If we have a tool that translates the rules, with a few exceptions, why not use it with the latest windup-rulesets build?

fabianvf commented 9 months ago

I think our initial plan to lock in an early release seems a bit optimistic in hindsight, considering the unexpected number of contributions we've been asked to pull in. Avoiding an endless cycle of content transfer and encouraging people to learn the new format is crucial, but we also don't want to slow down adoption by missing out on key rules in our repo.

I think there are basically two ways forward, given the assumption that we do want to support at least some new content from windup

  1. Tracking Windup Versions: Keeping up with the latest from Windup is a simple process and keeps us updated. This will mean our 80% goal will either be pinned to a moving target, or we drop the guarantee (it's especially affected by new groovy rulesets being added, which we can't support at this moment)
  2. Automating Rule Integration: Developing a system for specifying Git coordinates and automating the integration process. It saves time (especially for those like Juanma doing manual conversions) and lets us pick and choose what we integrate with precision. We can include the rulesets that are asked for, and avoid pulling in groovy ones. This would be my preference I think
    • Testing Strategy: The only downside to this approach would be that we lose the windup testing for the targeted rulesets we're bringing in. I think this could be a great motivation to finally create our own testing format/process and convert Windup tests into ours. When we run the conversions for targeted rulesets, we could bring the tests in with them, and have them run as part of the rulesets CI. I think this would be a win/win, we'd get a testing framework for our system, pull in everything we need from windup, and avoid artificially decreasing our pass rate by pulling in new rulesets that we know we can't support
aufi commented 4 months ago

A lot of CI enhancement work has been done in last months including things mentioned on this issue, closing as solved.