feat(registry): Require and validate registry metadata on first publish#777
feat(registry): Require and validate registry metadata on first publish#777
Conversation
|
![sentry[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
#skip-changelog
s1gr1d
force-pushed
the
sig/registry-docs-url
branch
from
2d2b2fd to
3a8acc3
Compare
![cursor[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
However, we cannot fail if something is missing because there might always be packages where a certain manifest configuration is allowed (e.g. without a package_url).
The problem is that prior to craft supporting adding the first release registry entry, the instructions for manually adding a new package to the registry specified these fields to be set. I'm assuming that so far we always stuck to this (?). At least for main_docs_url it seems we did, otherwise we would have run into the incident sooner.
As we saw with INC-2084, this implicitly means these fields can always expected to be present by API consumers. I'm not sure if we adhere to this in every entry but I'm afraid that if we don't require the fields via Craft for the first publish, we run into similar situations again. The release registry API is consumed by Sentry, Docs, Sentry CLI, and probably some other projects. So not sure which other consumers might assume that all of these properties are present.
So bottom line, I think that we should fail for the first publish if any properties are missing, but not for any already published packages. Fully realize though that I might lack some context here. @s1gr1d @BYK wdyt?
Strong agree. We started using release registry information in |
perform SDK validation for new packages
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for fixing! LGTM from my end!
| | `name` | Your `.craft.yml` config. Applied on every publish, can be updated at any time | | ||
| | `package_url` | Your `.craft.yml` config. Applied on every publish, can be updated at any time | | ||
| | `main_docs_url` | Your `.craft.yml` config. Applied on every publish, can be updated at any time | | ||
| | `api_docs_url` | Your `.craft.yml` config. Applied on every publish, can be updated at any time | |
There was a problem hiding this comment.
We need the mapping for these from the .craft.yml config. (main_docs_url -> mainDocsUrl etc.)
There was a problem hiding this comment.
Added two columns with both variable names 👍
3 participants
A package was published to the release registry without
main_docs_urlset. Now, we userepo_urlasmainDocsUrlfallback and emit warnings on missing metadata.Related Sentry PR: getsentry/sentry#110902
This PR adds upfront validation warnings and clearer, rewritten documentation for all config options (the diff looks big as I ran prettier).
Validation
For all new packages (apps, SDKs), it checks:
namemainDocsUrl(can also use the fallback value ofrepo_url)When an
sdkNameis provided, we can safely assume that a new SDK will be added and we additionally check forpackageUrl.In case of missing options, the workflow will fail -> avoids incorrect registry states.