Skip to content
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 to 2.3.7 #61

Merged
merged 8 commits into from May 8, 2019

Conversation

Projects
None yet
4 participants
@lapineige
Copy link
Member

commented Feb 20, 2019

Problem

  • This app is not up to date

Solution

  • Upgrade to 2.3.7

PR Status

  • Code finished.
  • Tested with Package_check.
  • Fix or enhancement tested.
  • Upgrade from last version tested.
  • Can be reviewed and tested.

Validation


Minor decision

  • Upgrade previous version :
  • Code review : JimboJoe
  • Approval (LGTM) : JimboJoe
  • Approval (LGTM) : Maniack C
  • CI succeeded :
    Build Status
    When the PR is marked as ready to merge, you have to wait for 3 days before really merging it.

lapineige added some commits Feb 20, 2019

@lapineige lapineige requested a review from JimboJoe Feb 20, 2019

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

Did you try it?
I tried it a couple of weeks ago, and LDAP wasn't working any more without any more clue on why 😢

@lapineige

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

No I didn't have the time.

I'll take a look.

@lapineige

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Just tested an upgrade: it works.

lapineige added some commits Apr 19, 2019

@lapineige lapineige changed the title Update to 2.3.6 Update to 2.3.7 Apr 19, 2019

@lapineige

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Now I tried directly with version 2.3.7, it works too.

@JimboJoe could you try again just in case ?

@lapineige lapineige removed the work needed label Apr 19, 2019

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

SSO seems to work for me, but part of the custom patches fails to get applied...

77055 DEBUG + for p in $YNH_CWD/../sources/patches/${source_id}-*.patch
77057 DEBUG + patch -p1
77059 DEBUG (Stripping trailing CRs from patch; use --binary to disable.)
77060 DEBUG patching file app/config/services.yml
77064 DEBUG (Stripping trailing CRs from patch; use --binary to disable.)
77065 DEBUG patching file app/config/security.yml
77066 DEBUG Hunk #1 FAILED at 13.
77067 DEBUG 1 out of 2 hunks FAILED -- saving rejects to file app/config/security.yml.rej
77068 DEBUG + for p in $YNH_CWD/../sources/patches/${source_id}-*.patch
77069 DEBUG + patch -p1
77070 DEBUG (Stripping trailing CRs from patch; use --binary to disable.)
77072 DEBUG patching file app/config/security.yml
77074 DEBUG Hunk #1 succeeded at 54 (offset -6 lines).
77075 DEBUG (Stripping trailing CRs from patch; use --binary to disable.)
77080 DEBUG patching file app/config/services.yml
77081 DEBUG (Stripping trailing CRs from patch; use --binary to disable.)
77082 DEBUG patching file src/Wallabag/YunoHostBundle/Security/LogoutSuccessHandler.php
77083 DEBUG + for p in $YNH_CWD/../sources/patches/${source_id}-*.patch
77084 DEBUG + patch -p1
77085 DEBUG patching file vendor/friendsofsymfony/oauth-server-bundle/Storage/OAuthStorage.php
77086 DEBUG Hunk #1 succeeded at 167 (offset 1 line).
@lapineige

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Hum, I never took a look at those patches, do you know how they are working ?
We don't have any extra information about what's not working ?

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

The patch part that fails relates to LDAP authentication... Did you try using the Firefox plugin and the Android application?

@lapineige

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Oh no I did not (this is my test instance, not the main one, so I didn't try that).
I can't test the android app by the way.

So we need to figure out why this patch doesn't work.

@lapineige

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Ok, the Firefox Extension works, as well as android app: https://forum.yunohost.org/t/official-app-wallabag2/3966/10

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I've commited patches update: no more patching problems.
Upgraded my personal instance: working OK, including Firefox plugin and Android app 👍

@lapineige

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

Let's merge then ?

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Nope, let's respect the workflow for official apps 😉

@JimboJoe
Copy link
Contributor

left a comment

LGTM and code review OK 👍

@@ -8,7 +8,7 @@
},
"url": "https://www.wallabag.org",
"license": "MIT",
"version": "2.3.2-1",
"version": "2.3.7",

This comment has been minimized.

Copy link
@JimboJoe

JimboJoe May 5, 2019

Contributor
Suggested change
"version": "2.3.7",
"version": "2.3.7~ynh1",

This comment has been minimized.

Copy link
@lapineige

lapineige May 5, 2019

Author Member

What does that change ?

This comment has been minimized.

Copy link
@JimboJoe

JimboJoe May 5, 2019

Contributor

You respect official packaging rules 😛

This comment has been minimized.

Copy link
@JimboJoe

JimboJoe May 5, 2019

Contributor

Don't forget to commit that change 😉

@lapineige

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

(does this category still exist ? 😋)

This is just merging into testing, not the master. And for a minor version, waiting for almost 3 months now.
We'll do that for the testing->master merge.

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

If you have suggestions regarding the workflow, you can propose them to the community. In the meanwhile, they're are applicable rules... Let's keep it the democratic way 😉
And by the way, you didn't get attention from the Apps group, as you didn't request a review from the team 😉

@JimboJoe JimboJoe requested a review from YunoHost-Apps/apps-group May 5, 2019

@lapineige

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

So we need to call for an additional round of testing from users ?

I installed that upgrade on 2 different servers (in 2 different ways), you did it too, CI is succeeding, some users reported to me that was working… And yet it is not a merge to master 😄
(I kind of want to get rid of those patches that are lying around and asking maintainers to think about coming back later and taking care of them all the time, as you can see ^^)

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I understand your frustration for often living it. Yet, I changed the code this morning, and it hasn't been tested by anyone except me. Furthermore, merging into master will always require reviews.
And you really don't want to experience too often the breaking of multiple users instance due to your merging 😨

@lapineige

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

and it hasn't been tested by anyone except me

I just did it :)

And you really don't want to experience too often the breaking of multiple users instance due to your merging 😨

Yeah, clearly, but that counts for master, people using testing know that this might break there instance, and that their own risks.
And in that case, that allow them to use 2.3.7, and maybe test fail2ban with it.

Furthermore, merging into master will always require reviews.

Yeah, obviously. I just mean I don't feel like testing require that much precaution. (by the way I didn't know we had the same validation process for testing as for master)

Let's wait for more testers then :)

@JimboJoe

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

That will be better than that. I propose you wait for the approval and merging into testing of this PR, #65 and #67 (requires apps group validation), so it then will have much sense to test all these PR together in a testing release!

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Globally agree with JimboJoe.

Can be merged in 3 days.

@lapineige lapineige merged commit 9649bdc into YunoHost-Apps:testing May 8, 2019

@nicofrand

This comment has been minimized.

Copy link

commented May 10, 2019

Sorry, I did not have time to test it before, so I just upgraded by wallabag version on my YNH with the testing branch, I had no issue, everything worked as expected! Thx

@lapineige

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

So that counts for #65 too, great :)

@maniackcrudelis maniackcrudelis referenced this pull request May 13, 2019

Merged

Testing #70

7 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.