-
Notifications
You must be signed in to change notification settings - Fork 4
Adjust notifier to Nextcloud 17 #10
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
Conversation
Signed-off-by: Joas Schilling <[email protected]>
|
info.xml needs a bump as well: preferred_providers/appinfo/info.xml Line 15 in 2284132
|
|
which also means there has to be a new release available before 17, as well as 17 should be removed from the old stable branch and a new version be uploaded |
Correct. |
|
@skjnldsv could you have a look? |
skjnldsv
left a comment
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.
Code looks good :)
| <bugs>https://github.com/nextcloud/preferred_providers</bugs> | ||
| <dependencies> | ||
| <nextcloud min-version="14" max-version="17"/> | ||
| <nextcloud min-version="17" max-version="18"/> |
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.
Claiming to be compatible with a version that is not even started yet? did you misunderstand the max-version? Or what's the reason behind it? if you have max-version="17" it will work with all 17 versions.
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.
Because I want to be able to work with it in 18?
As soon as we branch off on server, this will be compatible with 17 and 18.
This pr will not be merged until 17 is released and branched off :)
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.
Well but it defeats totally the porpuse of the field if you put it in the app store like this.
Same as before. the old version in the app store says it's compatible with 17, but it isnt 🤷
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.
Right, this release is a mistake :)
So I should just do min 17 max 17, right?
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.
Yes, and after stable17 is branched of you increase it in master
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.
Thanks @nickvergessen :)
|
Then this is now good to go and ready for a release right? |
|
SHould be yes :) |
9b5f68f to
9b11d17
Compare
9b11d17 to
3f8c786
Compare
Ref nextcloud/server#15040
fix #12