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.

Sign up for GitHub

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

Jump to bottom

fix: update testing range for 25, 26 and 27, and update dependencies #475

Merged
merged 11 commits into from Jun 12, 2023
Merged

fix: update testing range for 25, 26 and 27, and update dependencies #475

merged 11 commits into from Jun 12, 2023

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 7, 2023

No description provided.

@skjnldsv skjnldsv requested review from blizzz and come-nc June 7, 2023 06:42
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 7, 2023

@blizzz what is the usual procedure for this?
Do we actually need to test upgrades here too?

@blizzz
Copy link
Member

blizzz commented Jun 7, 2023

@blizzz what is the usual procedure for this? Do we actually need to test upgrades here too?

Who is the "we" you are thinking of? Release management? Then, I do not recall that was ever a topic for the handover to me, and technically i see this not strongly bound to release management. It's a software component like any other, that should have a maintainer and backup (currently no one assigned). We are not responsible for migration code of any other app, either, or their PHP compatibility. This does not say we could not be maintaining it in principle.

With another step back, without any fundamental changes to go for, we indeed can add this into the process and keep the test configurations up to date, along the other side tasks. Having this an item during branch-off would be an opportunity. Should be included in the release todos.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 8, 2023 •
edited

I meant we as Nextcloud ahah
I did not even know we had tests like those here. :/

@AndyScherzinger, do we know who's in charge of this repo?

@come-nc
Copy link
Collaborator

come-nc commented Jun 8, 2023

I meant we as Nextcloud ahah I did not even know we had tests like those here. :/

@AndyScherzinger, do we know who's in charge of this repo?

From what I remember the tests seemed to be for testing the updater more than the actual upgrade, and were not that easy to update. But I do not remember what was not easy 🤔

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 8, 2023

We could have an autopated script that:

  1. Pulls the updated config
  2. Reads min php and non-EOL versions
  3. Builds the updater
  4. Tries an upgrade

@come-nc come-nc self-assigned this Jun 8, 2023
skjnldsv and others added 8 commits June 8, 2023 17:57
@skjnldsv @come-nc
fix: update updater testing range for 25, 26 and 27
91a968e 
Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@skjnldsv @come-nc
fix: add 8.2 php and fix actions hashes
903820d 
Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@skjnldsv @come-nc
fix: actions hashes
2713b23 
Signed-off-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@come-nc
Adapt Makefile and signatures to test using newer versions
8586034 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix test-cli tests as well
3450e81 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@skjnldsv @come-nc
fix: update index.php and updater.phar
e60627e 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv @come-nc
fix: update composer deps and box build workflow
3302797 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@come-nc
Install dependencies in workflows
b3b17cf 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Collaborator

come-nc commented Jun 8, 2023

I merged #477 here to see if both combine fixes CI.

@AndyScherzinger
Copy link
Member

@AndyScherzinger, do we know who's in charge of this repo?

currently nobody 😢 - I added it to the list of apps where we need to assign ownerships (sheet we already have in general)

@come-nc
Make sure all CI steps can run on PHP 8.0
ef3a0cd 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Test beta update with cleaner beta
b8f9d72 
24.0.0beta2 has extra files which causes updater to complain. Moved the
   test to stable26.feature and use betas without extra files.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@szaimen
Copy link
Contributor

szaimen commented Jun 12, 2023

test-master should be fixable by adjusting:

updater/tests/features/master.feature

Line 10 in 65958c0

Then the installed version should be 26.0

@come-nc
Master is now 28
01d3a26 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc changed the title fix: update updater testing range for 25, 26 and 27 fix: update testing range for 25, 26 and 27, and update dependencies Jun 12, 2023
@come-nc come-nc requested review from kesselb and szaimen June 12, 2023 12:35
szaimen
szaimen reviewed Jun 12, 2023
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small remarks

.github/workflows/test-stable.yml
@@ -16,25 +16,31 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
php-versions: ["7.4", "8.0"]
nextcloud-versions: ["19", "20", "21"]
php-versions: ["8.0", "8.1", "8.2"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not test 24 and 25 with php 7.4?

tests/features/stable24.feature

Scenario: Update is available - 24.0.1 to 25.0.0
Given the current installed version is 24.0.1
And PHP is at least in version 8.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt it 7.4?

tests/features/stable25.feature
And PHP is at least in version 7.3
Scenario: Update is available - 25.0.0 to beta
Given the current installed version is 25.0.0rc1
And PHP is at least in version 8.0
Copy link
Contributor

@szaimen szaimen Jun 12, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt it be 7.4?

@come-nc
Copy link
Collaborator

come-nc commented Jun 12, 2023

@szaimen I missed that 24 and 25 did not drop 7.4 yet. Still this updater master branch will not be used to build 24.x and 25.x, right?

I can switch to 7.4 minimum, but that means changing all dependencies again and I’m afraid we may not be able to support both 7.4 and 8.2 in our dependencies.
Since master is supposed to work with nextcloud/server master, I feel fine dropping 7.4.

@szaimen
Copy link
Contributor

szaimen commented Jun 12, 2023

I missed that 24 and 25 did not drop 7.4 yet. Still this updater master branch will not be used to build 24.x and 25.x, right?

Yes

I can switch to 7.4 minimum, but that means changing all dependencies again and I’m afraid we may not be able to support both 7.4 and 8.2 in our dependencies. Since master is supposed to work with nextcloud/server master, I feel fine dropping 7.4.

All right. Then lets leave it like this

szaimen
szaimen approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but didnt test

@come-nc
Copy link
Collaborator

come-nc commented Jun 12, 2023

@skjnldsv Can you merge this in master? It seems merging is restricted here.

@blizzz blizzz merged commit aac9e4b into master Jun 12, 2023
22 checks passed
@blizzz blizzz deleted the skjnldsv-patch-1 branch June 12, 2023 14:47
@szaimen
Copy link
Contributor

szaimen commented Jun 12, 2023

Thanks @blizzz 💙

@come-nc come-nc mentioned this pull request Sep 14, 2023
Merged
@come-nc come-nc mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blizzz blizzz blizzz approved these changes

@szaimen szaimen szaimen approved these changes

@come-nc come-nc Awaiting requested review from come-nc

@kesselb kesselb Awaiting requested review from kesselb

Assignees

@come-nc come-nc

Labels
4. to release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@skjnldsv @blizzz @come-nc @AndyScherzinger @szaimen