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

Deprecate package metadata, improve packaging documentation, refactor record package schema #1640

Merged
merged 25 commits into from
Nov 7, 2023

Conversation

duncandewhurst
Copy link
Contributor

@duncandewhurst duncandewhurst commented Oct 8, 2023

@duncandewhurst duncandewhurst changed the title Deprecate package metadata, improve packaging documentation Deprecate package metadata, improve packaging documentation, refactor record package schema Oct 13, 2023
@duncandewhurst
Copy link
Contributor Author

duncandewhurst commented Oct 13, 2023

Regarding the test failures:

test_json.py::test_schema_valid[/home/runner/work/standard/standard/schema/record-package-schema.json-record-package-schema.json-data4]
  schema/record-package-schema.json has Error while resolving `[https://standard.open-contracting.org/schema/1__2__0/record-schema.json`:](https://standard.open-contracting.org/schema/1__2__0/record-schema.json%60:) HTTPError: 404 Client Error: Not Found for url: https://standard.open-contracting.org/schema/1__2__0/record-schema.json at properties/records/items

This will be resolved once 1__2__0 is released, I'm not sure what to do about it in the meantime. As a result, the record package schema browser does not display.

test_json.py::test_schema_valid[/home/runner/work/standard/standard/schema/record-schema.json-record-schema.json-data0]
  schema/record-schema.json includes "null" in "type" at /definitions/LinkedRelease/properties/url

test_json.py::test_schema_valid[/home/runner/work/standard/standard/schema/record-schema.json-record-schema.json-data0]
  schema/record-schema.json is missing "null" in "type" at /definitions/LinkedRelease/properties/tag

test_json.py::test_schema_valid[/home/runner/work/standard/standard/schema/record-schema.json-record-schema.json-data0]
  unused codelists: awardCriteria.csv, awardStatus.csv, classificationScheme.csv, contractStatus.csv, country.csv, currency.csv, documentType.csv, extendedProcurementCategory.csv, initiationType.csv, language.csv, linkRelationType.csv, mediaType.csv, method.csv, milestoneStatus.csv, milestoneType.csv, partyRole.csv, partyScale.csv, procurementCategory.csv, relatedProcess.csv, relatedProcessScheme.csv, submissionMethod.csv, tenderStatus.csv, unitClassificationScheme.csv

I think the tests need to be updated to add exceptions for the new record schema since these warnings were not previously reported when the Record definition was embedded in the record package schema:

@duncandewhurst duncandewhurst marked this pull request as ready for review October 13, 2023 01:45
@jpmckinney
Copy link
Member

jpmckinney commented Oct 13, 2023

Hmm, url being required but nullable seems like an error/oversight. A linked release is not at all useful without a URL. Let's remove null.

Edit: I made some changes. The other errors shouldn't occur again (hard to test the 404 fix locally).

Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few fixes.

Need to delete the two changelog entries for #1422. Also missing changelog entries for:

  • Deprecations (the deprecated fields and any sub-fields are no longer required)
  • records and releases are no longer required to be unique

Governance-wise, we can't justify the present with the future, so I changed the deprecation notices.

Please make identical or corresponding changes to the release package schema and page.

schema/record-package-schema.json Outdated Show resolved Hide resolved
schema/record-package-schema.json Outdated Show resolved Hide resolved
schema/record-package-schema.json Outdated Show resolved Hide resolved
schema/record-package-schema.json Outdated Show resolved Hide resolved
schema/release-package-schema.json Outdated Show resolved Hide resolved
docs/schema/packaging/record_package.md Show resolved Hide resolved
docs/schema/packaging/record_package.md Outdated Show resolved Hide resolved
docs/schema/reference.md Outdated Show resolved Hide resolved
docs/schema/record.md Outdated Show resolved Hide resolved
docs/schema/record.md Outdated Show resolved Hide resolved
@duncandewhurst
Copy link
Contributor Author

Hmm, url being required but nullable seems like an error/oversight. A linked release is not at all useful without a URL. Let's remove null.

Done in 80d245b

Edit: I made some changes. The other errors shouldn't occur again (hard to test the 404 fix locally).

The 404 error still occurs

Need to delete the two changelog entries for #1422.

Done in 710ab21

Also missing changelog entries for:

* Deprecations (the deprecated fields and any sub-fields are no longer required)

* records and releases are no longer required to be unique

Done in d4e6897

Governance-wise, we can't justify the present with the future, so I changed the deprecation notices.

Please make identical or corresponding changes to the release package schema and page.

Done in 3d1b39d and 5b3358e

…events set GITHUB_BASE_REF, needed by test_json.py)
@jpmckinney
Copy link
Member

jpmckinney commented Nov 3, 2023

The 404 error still occurs

Ah, test_json.py was trying to use GITHUB_BASE_REF to determine whether the PR was for an unreleased tag or not (e.g. if the base ref contained "1.2", like 1.2-dev), but that environment variable is only available if the workflow is triggered by the pull_request event. The workflow has an if condition in order to not run twice for both the push and pull_request events (it would always run for push, and only for pull_request if the PR was on a fork).

I've removed that if condition. That said, I expect the PR will still fail on the push event, but it should pass on the pull_request event. Edit: I now skip the push event on PRs.

Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

@duncandewhurst I added a few more commits to cut other content about the deprecated fields. If you agree, you can merge.

docs/schema/packaging/release_package.md Outdated Show resolved Hide resolved
Co-authored-by: James McKinney <26463+jpmckinney@users.noreply.github.com>
@duncandewhurst
Copy link
Contributor Author

The new commits look good!

@duncandewhurst duncandewhurst merged commit e778a95 into 1.2-dev Nov 7, 2023
10 checks passed
@duncandewhurst duncandewhurst deleted the 1621-deprecate-package-metadata branch November 7, 2023 01:54
@jpmckinney jpmckinney added this to the 1.2.0 milestone Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
2 participants