-
Notifications
You must be signed in to change notification settings - Fork 178
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
Collection assets to core #1008
Conversation
Ok, I think this is ready for review. @m-mohr - I hacked through schema changes, so hopefully I did them right. Thought I got myself into a failing state, but I think I figured it out. But would be good for you to check it over. I did click on 'this is not a breaking change', because I think it is in a very small way? If implementations said collection-assets for the extension then that will fail with it gone (though that will be true for every extension we move out). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a couple of changes:
- Remove extension from Code Owners file
- Align collection and item spec a bit more (i.e. moved assets details to "Additional Fields Information")
- As it was the last spec where it was missing, added h4 headers for "Additional Fields Information" in Items
- Add assets to examples (which may conflict with Examples refresh #955, but then it reminds us to check whether there are collection assets in the examples an add them if not)
- Added to changelog that "No change is required except removing
collection-assets
from the list ofstac_extensions
."
Please also review my changes @cholmes and feel free to change things again.
…c a bit more, add assets to examples
6eb8f0c
to
d958041
Compare
Cool. Just looked it over, changes look good. A couple comments:
I think I made these changes in another PR too, but we can just merge here and I'll deal.
#955 got merged - I added #1013 to track this though, as I agree we need to be sure to get them in. |
Related Issue(s): #1002
Proposed Changes:
PR Checklist: