-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update dl_vlan validation to allow mask #135
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.
@ajoaoff looks good to me.
Could you update the changelog? Also, could you provide either a migration MongoDB command in the changelog itself if it's short, or a migration script updating/mapping dl_vlan
values as string? This change would break anything but it would leave different types in the collection that can become confusing, 2022.3.X
will be used in production at AmLight, thanks.
@viniarck I'll provide the migration script, but anyway I don't think this PR should ship with |
@ajoaoff, yes, we won't ship to Antonio, if you could also update the Regarding a new branch for |
This PR also handles |
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.
LGTM. Thanks for also pushing this script @ajoaoff.
Antonio, before merging, let's also update the changelog, if you can also mention there about this script that'd be great. @italovalcy when AmLight upgrades to 2023.1
some months down the road you'll probably want to run this script.
d5b31f1
to
5a4277d
Compare
5a4277d
to
168c9c3
Compare
@viniarck and @ajoaoff, since this is a major change that will impact other Napps, IMO we have to increase the API version and keep the old one compatible with the previous format—maybe providing an abstraction between the two versions (which will convert int to string). Another strategy would be supporting both int and string. WDYT? |
Good point. If we decide to unify the string format for |
Agreed @italovalcy @Alopalao, I tend to avoid mixing different type for the same field Regarding bumping v3 though, let's leave it to when we address the other validations in general on issue #43 that we have on our radar ( |
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.
Closes #132
dl_vlan
in the formatvlan/mask
in addition tovlan