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

[DO NOT MERGE][WORK IN PROGRESS]kie-issues#1613: Add CI :: Check license headers GitHub Actions workflow #2746

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

jomarko
Copy link
Contributor

@jomarko jomarko commented Nov 15, 2024

This is partial fix of the issue apache/incubator-kie-issues#1613 as we open one PR per repository.

This is partial fix of the issue apache/incubator-kie-issues#1613 as we open one PR per repository.
.rat-excludes Outdated
**/META-INF/beans.xml
**/licenses/**

### kie-tools specific excludes
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 probably most complicated repository we have, what I need from reviewers is to help with finalization of the exclude patterns, once done and we are sure all what is needed is excluded from 'apache-rat-check', I will fix those files == add license there.


- name: Download Apache RAT
run: |
curl -LO https://repository.apache.org/content/repositories/snapshots/org/apache/rat/apache-rat/0.17-SNAPSHOT/apache-rat-0.17-20241115.065104-374.jar
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using a SNAPSHOT version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, however the stable version 16 does not support glob pattern in the '.rat-excludes' file, that is why I used 17-SNAPSHOT.

My idea was to use this specific snapshot version until there is stable/final 17, we could track this in kie-issues ticket?

If you say we need to use stable/final 16, we need to list every single excluded file in .rat-excludes

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.... understand... I'm not a big fan os glob exclusions, to be honest, as I think they make it too easy to add stuff to the excluded list. Idk, first I think we need an assessment of which files we should be excluding from "CI :: Check license headers", then I think we can discuss the best approach to do it... WDYT please?

Copy link
Contributor Author

@jomarko jomarko Nov 19, 2024

Choose a reason for hiding this comment

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

Ok, I will prepare a spreadsheet with files, we need to discuss, FYI, even excluding:

**/target/**
**/node_modules/**
**/dist/**
**/dist-dev/**
**/dist-tests/**
**/dist-tests-e2e/**
**/python-venv/**

we need to discuss ~995 files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shared privately

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't be necessary to include anything that is in the .gitignore...

Copy link
Contributor

@tkobayas tkobayas Nov 20, 2024

Choose a reason for hiding this comment

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

Hi,

I'm not a big fan os glob exclusions, to be honest, as I think they make it too easy to add stuff to the excluded list

apache-rat 0.16 uses filename with regex e.g. .*\.csv. I think "easy to add" is not so different between apache-rat 0.16 (regex) and 0.17 (glob pattern). Rather, apache-rat 0.17 glob pattern can express file paths, while apache-rat 0.16 cannot express things like **/optaplanner-examples/data/**/import/**. I'll ask about the preference in dev ML.

I'm not against the assessment of excluding files at all. Thanks!

@tiagobento tiagobento changed the title kie-issues#1613: Add apache-rat check github action kie-issues#1613: Add Apache RAT check GitHub Actions workflow Nov 15, 2024
Copy link
Contributor

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

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

Thank you for starting this effort @jomarko ! I made some suggestions for us to focus on the "what" (Checking license headers), instead of the "how" (Apache RAT).

.github/workflows/pr-rat-check.yml Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Please name this ci_check_license_headers.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 22 to 24
on:
pull_request:
types: [opened, synchronize, reopened, ready_for_review]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think making it run on merges to main would be good too... We can use the same triggers we have for ci_build.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.github/workflows/pr-rat-check.yml Outdated Show resolved Hide resolved
@tiagobento tiagobento changed the title kie-issues#1613: Add Apache RAT check GitHub Actions workflow kie-issues#1613: Add CI :: Check license headers GitHub Actions workflow Nov 15, 2024
.rat-excludes Outdated
packages/stunner-editors/kie-wb-common-stunner/kie-wb-common-stunner-sets/kie-wb-common-stunner-bpmn/kie-wb-common-stunner-bpmn-emf/src/main/java/org/eclipse/dd/di/impl/StyleImpl.java
packages/stunner-editors/kie-wb-common-stunner/kie-wb-common-stunner-sets/kie-wb-common-stunner-bpmn/kie-wb-common-stunner-bpmn-emf/src/main/java/org/eclipse/dd/di/util/DiAdapterFactory.java
packages/stunner-editors/kie-wb-common-stunner/kie-wb-common-stunner-sets/kie-wb-common-stunner-bpmn/kie-wb-common-stunner-bpmn-emf/src/main/java/org/eclipse/dd/di/util/DiSwitch.java
packages/stunner-editors/kie-wb-common-stunner/kie-wb-common-stunner-sets/kie-wb-common-stunner-bpmn/kie-wb-common-stunner-bpmn-emf/src/main/java/org/eclipse/dd/di/util/DiValidator.java
Copy link
Contributor

Choose a reason for hiding this comment

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

listing every excluded single file is nice. However, this format (file path) doesn't work with apache-rat 0.16.1. Please confirm.

Per my testing so far, apache-rat 0.16.1 supports file name without its file path (aaa.txt) or directory name (ccc/*). So far, my idea is only listing file names to exclude (It means listing like DiValidator.java, so it would affect all DiValidator.java in any paths). If you find another way to express file paths, it would be great.

Another idea is to fall back to apache-rat 0.17 and listing every excluded single file as this PR. The problem of "snapshot is pruned" could be resolved by hosting the file somewhere (e.g. incubator-kie-kogito-pipelines)... it may not sound good though.

.rat-excludes Outdated
packages/serverless-workflow-diagram-editor/uberfire-extensions/uberfire-commons-editor/uberfire-commons-editor-client/src/main/resources/org/uberfire/ext/editor/commons/client/file/exports/js/FileSaver.min.js.back
packages/serverless-workflow-diagram-editor/uberfire-extensions/uberfire-commons-editor/uberfire-commons-editor-client/src/main/resources/org/uberfire/ext/editor/commons/client/file/exports/js/canvas2svg.js.back
packages/stunner-editors/errai-common/src/main/java/org/jboss/errai/common/client/util/TimeUnit.java
packages/stunner-editors/errai-common/src/main/java/org/jboss/errai/common/compat/javax/annotation/processing/Generated.java
Copy link
Contributor

@tkobayas tkobayas Nov 21, 2024

Choose a reason for hiding this comment

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

@porcelli Per https://lists.apache.org/thread/nrq50szwx37doopb23wyq33v0t3bbccg , I think Generated.java needs to be listed in LICENSE as Category B or X. But if it's one time listing for 10.0.0, it may not be needed in main. Thoughts?

.rat-excludes Outdated
packages/stunner-editors/kie-wb-common-dmn/kie-wb-common-dmn-webapp-kogito-marshaller/src/main/resources/DI.xsd
packages/stunner-editors/kie-wb-common-dmn/kie-wb-common-dmn-webapp-kogito-marshaller/src/main/resources/DMN12.xsd
packages/stunner-editors/kie-wb-common-dmn/kie-wb-common-dmn-webapp-kogito-marshaller/src/main/resources/DMNDI12.xsd
packages/stunner-editors/kie-wb-common-stunner/kie-wb-common-stunner-sets/kie-wb-common-stunner-bpmn/kie-wb-common-stunner-bpmn-emf/src/main/java/org/eclipse/bpmn2/Activity.java
Copy link
Contributor

Choose a reason for hiding this comment

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

@porcelli @jomarko Ohh, these files with org/eclipse package classes has EPL license which is Category B. They need to be in LICENSE, right?

(I haven't fully checked the source codes. Just reported what I happened to find)

@jomarko jomarko added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Nov 21, 2024
@jomarko jomarko changed the title kie-issues#1613: Add CI :: Check license headers GitHub Actions workflow [DO NOT MERGE][WORK IN PROGRESS]kie-issues#1613: Add CI :: Check license headers GitHub Actions workflow Nov 21, 2024
@tiagobento
Copy link
Contributor

@jomarko Sorry for being picky, but to to make this absolutely perfect (😄), can we rename the workflow to CI :: License headers and the name of the job from check-license-headers to simply check?

This way, instead of

image

we'll have

image

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:cicd area:monorepo pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants