-
Notifications
You must be signed in to change notification settings - Fork 0
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
[LDR] hubverse target_keys
should contain a single value
#7
Conversation
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.
looks good, thanks for putting it together! had a question about version number. Am approving in case you believe the version number is correct.
We will update our schemas to specify that `target_keys` should contain exactly | ||
one property if it is not `null`. | ||
|
||
We will increment the version of the schemas to v4.0.1. |
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.
Should this be regarded as a major version change, in the sense that if a hub previously had multiple target keys, they can't just update to the latest version and still have a working schema? (This is a real question, not a suggestion to change the version increment.)
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 think that's a good question. My opinion is that it should be a major version change because of this reason.
It's one of the slightly frustrating things that happens with semantic versioning: major version changes often feel like they should be earth-shattering changes, but they can be as small as removing an argument or (in this case) restricting a previously boundless field.
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.
That being said, given that I am recording a decision that was made in a meeting two weeks ago AND because the steps for implementation are already in progress, it's more appropriate to merge this in and then create another LDR to amend this decision if we think it's appropriate.
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 included this in the patch version I had already queued up because from what I understand no one was using this feature. It would not be a problem to bump to 5.0.0 for more semantic accuracy, would just need to change the version in the schema PR and a few snapshots in hubAdmin
.
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.
- Zhian and I looked back at our meeting notes, and we had basically said, "we don't think anyone is using this feature, but it would be good to send out an email to double check with existing hubs before going ahead with making this change"
- I don't recall having a specific discussion or decision about impact on version numbers when this first came up. If indeed no one is using this, I'd be ok with bending the semantic versioning rules.
I have added an LDR to reflect the decision made at the hubverse devteam meeting on 2024-12-11.
Here is the transcript of the discussion:
I am recording this as an LDR so I can refer to this decision for communications
PRs are already underway:
target_keys
object to contain only a single key value pair. hubverse-org/schemas#118