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

Check for common.licenseInformation #593

Closed
wants to merge 1 commit into from
Closed

Conversation

klein0r
Copy link

@klein0r klein0r commented Feb 22, 2024

@klein0r
Copy link
Author

klein0r commented Feb 22, 2024

Like this @foxriver76 ?

@foxriver76
Copy link
Contributor

LGTM, but will of course fail if new attribute not provided, but if someone touches adapter should not hurt.

Question is if this is used by RepoChecker, then it would be annoying, because not urgent @mcm1957 do you know details?

@klein0r
Copy link
Author

klein0r commented Feb 22, 2024

but will of course fail if new attribute not provided

Like the schema check for io-package in my IDE 😄

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 22, 2024

@AlCalzone
PLEASE DO NOT MERGE in this variant

The check MUST NOT fail if new attribute is missing.
The check MUST pass if EITHER license OR licenseInformation (and possible both of them) are present.

Otherwise every dependabot PR will fail for older adapters.
And every small change will cause the tests to fail.
I will not be able to releases without additional effort new versions as tests will fail.

@Apollon77
As far as I remember the discussion yesterday devs MUST NOT Be forced to adapt io-package.json in the very near future as this has no benefit for free adapters at all and will only cause devs to be frustrated.

@mcm1957
Copy link
Contributor

mcm1957 commented Feb 22, 2024

LGTM, but will of course fail if new attribute not provided, but if someone touches adapter should not hurt.

Question is if this is used by RepoChecker, then it would be annoying, because not urgent @mcm1957 do you know details?

This test is used at GITHUB actions only.
This test MUST acceot old license attribute too, current change is to restrictive.

Repochecker is completly independent.
Repochecker must accpet old and new syntax too, a warnig if license is used is OK. No error must be issued.

@klein0r
Copy link
Author

klein0r commented Feb 22, 2024

The check MUST NOT fail if new attribute is missing. The check MUST pass if EITHER license OR licenseInformation (and possible both of them) are present.

In that case the schema should also be valid if one of the attributes is configured. At the moment licenseInformation is a required attribute and my IDE complains about the missing attribute... So I've changed it and removed the deprecated attribute.

Now all tests of all my adapters fail and I provided this PR which matches the schema of io-package (removed a deprecated attribute and check for the required attribute)...

@klein0r
Copy link
Author

klein0r commented Feb 22, 2024

In my opinion the schema is the leading instance here and all other processes (create-adapter, repochecker, testing, ...) have to follow that definition.

Copy link
Collaborator

@AlCalzone AlCalzone left a comment

Choose a reason for hiding this comment

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

This would break tests for all existing adapters - unlike the schema check in VSCode, which you can choose to ignore.

Until license is removed from js-controller or admin, we should enforce that at least one of license or licenseInformation exists.

create-adapter can be stricter, since it is used for new adapters.
IMO the repo-checker should only warn for now and guide devs to migrating from license to licenseInformation.

@klein0r
Copy link
Author

klein0r commented Feb 22, 2024

Ok then I'll wait for the official solution

@klein0r klein0r closed this Feb 22, 2024
@AlCalzone
Copy link
Collaborator

Happy to merge your PR if you modify it :)
That will probably be quicker than doing it myself.

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.

4 participants