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

Enable License Check for PDE #982

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Dec 5, 2023

@laeubi
Copy link
Contributor Author

laeubi commented Dec 5, 2023

/request-license-review

@laeubi laeubi merged commit 47e0d8c into eclipse-pde:master Dec 5, 2023
8 of 11 checks passed
@jukzi
Copy link
Contributor

jukzi commented Dec 5, 2023

@iloveeclipse
Copy link
Member

that breaks all builds now

Yep... Please fix or revert.

@HannesWell
Copy link
Member

We ware await the addition of a GITLAB_API_TOKEN for this organization by the EF-infra team:
https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4037

With that we can request the license vetting of the offending projects.

@HannesWell
Copy link
Member

/request-license-review

Copy link

github-actions bot commented Dec 5, 2023

/request-license-review

⚠️ Failed to request review of not vetted licenses.

Workflow run (with attached summary files):
https://github.com/eclipse-pde/eclipse.pde/actions/runs/7107312115

@laeubi
Copy link
Contributor Author

laeubi commented Dec 6, 2023

that breaks all builds now

Yep... Please fix or revert.

It does NOT break the build because it is not even part of the build. It adds a new check that currently fails because some unvetted artifacts have recently slipped in and that the purpose of this check, to fail so such things did not happen unnoticed.

So can we please calm down a bit instead and try to understand why things happen, if anyone is that concerned we can of course fall back to manually checks and creating CQs by hand.

@iloveeclipse
Copy link
Member

@laeubi : the problem I see is that adding actions that are known to fail distracts everyone for no obvious reason.
If the problem is known, why not wait till it is fixed and enable the action after that?

With every new action added that fails by default everyone learns to ignore github action errors and keep ignoring them in the future. This is https://wiki.c2.com/?FixBrokenWindows in pure form.

So we would be happy to see more checks and we value your work, but new checks should be introduced in a way that makes everyone happy.

@akurtakov
Copy link
Member

In this case the choices are:

As 3) is not an option and we can not fix 1) without 2) what we have here is the best forward looking path.

@akurtakov
Copy link
Member

One can read #954 (comment) for the detailed analysis and the request for exactly this one.

@akurtakov
Copy link
Member

Work towards fixing the licensecheck is ongoing in #983 . I am not aware of any other way to have this fixed without suffering short term.

@jukzi
Copy link
Contributor

jukzi commented Dec 6, 2023

i don't want to read through all the history, can you please summarize why such a change has to be submitted? As far as i understand such checks can also be used with not-submitted PRs, as each PR uses the workflows it submits.

@akurtakov
Copy link
Member

@jukzi https://docs.github.com/en/actions/using-workflows/about-workflows#about-workflows :

Workflows are defined by a **YAML file checked in ** to your repository and will run when triggered by an event in your repository, or they can be triggered manually, or at a defined schedule.

A PR is not checked in thus workflow in it will not be executed.

@laeubi
Copy link
Contributor Author

laeubi commented Dec 6, 2023

the problem I see is that adding actions that are known to fail distracts everyone for no obvious reason.

Some action require elevated permissions and can not run as part of a PR, so there is not much of an option, beside that the workflow fails because something is wrong in PDE repository not because the check itself is failing.

With every new action added that fails by default everyone learns to ignore github action errors and keep ignoring them in the future. This is https://wiki.c2.com/?FixBrokenWindows in pure form.

No this is maybe ignorance in pure form, please don't blame that on making things VISIBLE, the window is already BROKEN but no one has noticed it yet so please don't start this fruitless discussion again. There is no single line in the committer handbook that says one can ignore failures just because it annoyed anyone in the past.

i don't want to read through all the history, can you please summarize why such a change has to be submitted?

Please read the committer handbook about IP due diligence, if you feel that we should enforce each and every contribution to be checked by a committer manually then "we" don't need it, if we want to benefit from automatic checks and automatic request "we" need it.

jukzi added a commit that referenced this pull request Dec 6, 2023
@jukzi jukzi mentioned this pull request Dec 6, 2023
@jukzi
Copy link
Contributor

jukzi commented Dec 6, 2023

A PR is not checked in thus workflow in it will not be executed.

looks like a PR is treated to be checked in, see example
image
you also can see that "call-license-check / check-licenses" in this PR workflow was executed before it was checked in

@akurtakov
Copy link
Member

Because it already is as can be seen at https://github.com/eclipse-pde/eclipse.pde/blob/master/.github/workflows/licensecheck.yml . If there is no such workflow file in the repo it would not be executed.

@jukzi
Copy link
Contributor

jukzi commented Dec 6, 2023

If there is no such workflow file in the repo it would not be executed.

thats simply not true - you can see that "call-license-check / check-licenses" in this(982) PR workflow was executed before it was checked in

image

the same was true when i added codeQL

@akurtakov
Copy link
Member

akurtakov commented Dec 6, 2023

The requirement seems to be specific to (parts?) of this workflow eclipse-jdtls/eclipse.jdt.ls#2966 (comment)

@laeubi
Copy link
Contributor Author

laeubi commented Dec 6, 2023

thats simply not true - you can see that "call-license-check / check-licenses" in this(982) PR workflow was executed before it was checked in

Not everything is always that simple, if you have write access to the repository and the check does trigger on PR it will be executed.

The /request-license-review command (for what I opened https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/4037) can only be checked if it is part of the repository already.

Nerveless the check on PR fails because something is WRONG in PDE that got unnoticed and it should fail!
That is similar to adding a freeze-check while freeze period, you WANT that is gives a failure mark on the PR, this does not mean the workflow is wrong/failing....

So exactly that happens here, we noticed a license problem, I added a check and I want it to show that problem unless we where able to fix it so it never happens again in the future.

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