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

git update auto change to edge branch #3589

Merged
merged 13 commits into from May 15, 2021

Conversation

Alkarex
Copy link
Member

@Alkarex Alkarex commented Apr 17, 2021

For existing installations using automatic git update, checkout edge branch if it was still using master or dev.

For existing installations using automatic git update, checkout *edge* branch if it was still using *master* or *dev*.
@Alkarex Alkarex added this to the 1.18.1 milestone Apr 17, 2021
exec('git reset --hard FETCH_HEAD', $output, $return);
}

// Automatic change to the new name of edge branch since FreshRSS 1.18.0
Copy link
Member

Choose a reason for hiding this comment

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

This can be put in a migration instead to avoid more complicated code here

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, it worked for me 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, I am not familiar with this part of the code yet...

Copy link
Member

Choose a reason for hiding this comment

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

Something like that should work (not tested):

// File: app/migrations/2021_04_24_GitCheckoutFromMasterToEdge.php
class FreshRSS_Migration_2021_04_24_GitCheckoutFromMasterToEdge {
	public static function migrate() {
		if (!is_dir(FRESHRSS_PATH . '/.git/')) {
			return true; // not a Git installation, nothing to do
		}

		exec('git branch --show-current', $output, $return);
		if ($return != 0) {
			throw new Exception('Can’t checkout to edge branch, please change branch manually.');
		}

		$line = is_array($output) ? implode('', $output) : $output;
		if ($line !== 'master' && $line !== 'dev') {
			return true; // not on master or dev, nothing to do
		}

		unset($output);
		exec('git checkout edge --guess -f', $output, $return);
		if ($return != 0) {
			throw new Exception('Can’t checkout to edge branch, please change branch manually.');
		}

		unset($output);
		exec('git reset --hard FETCH_HEAD', $output, $return);
		if ($return != 0) {
			throw new Exception('Can’t checkout to edge branch, please change branch manually.');
		}

		return true;
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Here you are e09a1e9

Copy link
Member Author

@Alkarex Alkarex May 4, 2021

Choose a reason for hiding this comment

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

@marienfressinaud It does not look like the migration errors / results / messages are visible in FreshRSS logs though. Only the result of the git command is visible in the Web server logs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also have a case of white page when using Migration

@Alkarex
Copy link
Member Author

Alkarex commented May 8, 2021

Before we would get a white page in case of Migration errors. After a0be21d we get:

image

@Alkarex
Copy link
Member Author

Alkarex commented May 8, 2021

Ping @marienfressinaud

lib/lib_rss.php Outdated Show resolved Hide resolved
@Alkarex
Copy link
Member Author

Alkarex commented May 8, 2021

I am not super convinced of using this migration system compared to the first approach inside the update controller. The migration system has a high likelihood of breaking an installation, cannot be skipped / postponed, cannot be re-applied when the situation becomes relevant again, reduces performances at each query.
I will try to add a few more tests for the obvious problems for which the migration system bricks FreshRSS, such as when git does not exist, or when the access rights do not allow the Web server to update automatically, but then I do not know what to do because I do not want to test for those things at each request, and the migration has not passed.

@Alkarex
Copy link
Member Author

Alkarex commented May 8, 2021

Arg, another problem: we need to check Minz_Configuration::get('system')->disable_update but it is not yet available at the time the Migration system runs.
For instance, it currently breaks Docker deployments mounting the FreshRSS code as a volume.
@marienfressinaud I think I will move the logic back to the update controller, except if you see an easy path with the migration system?

@marienfressinaud
Copy link
Member

marienfressinaud commented May 8, 2021

I am not super convinced of using this migration system compared to the first approach inside the update controller. The migration system has a high likelihood of breaking an installation, cannot be skipped / postponed, cannot be re-applied when the situation becomes relevant again, reduces performances at each query.

The migration system is here to simplify the complexity of code by moving "update" part in a dedicated part of the application. Some parts are hard to understand and to maintain because they mix concepts.

I don't see why having this code in migrations increase the risk of breaking an installation. It's applied only once, just after getting the new code so it's quite similar to what you wrote first.

But I agree it would be better to don't have the migrations running on each request. Ideally it would be a separated command during update (could be applied automatically with the auto-update system, but should be manual otherwise).

Arg, another problem: we need to check Minz_Configuration::get('system')->disable_update but it is not yet available at the time the Migration system runs.
For instance, it currently breaks Docker deployments mounting the FreshRSS code as a volume.

Ah right, I didn't thought about that, but I don't understand why it would break such deployment. Actually I would think checking disable_update is not required since it should be applied after getting the new code, but I can miss something!

@marienfressinaud I think I will move the logic back to the update controller, except if you see an easy path with the migration system?

If it makes your life easier and you feel more confident with this solution, go ahead! I maintain it makes the code harder to understand/maintain, but you spend way more time maintaining it than me so don't worry too much about me ^^

@Alkarex
Copy link
Member Author

Alkarex commented May 15, 2021

@marienfressinaud I do like the idea of having a better and more uniform migration system. The key difference here is that if the new migration system fails, the whole FreshRSS installation is bricked until it is fixed (see screenshots above), compared with the approach inside the upgrade controller which will most likely just fail without consequence (and ability to easily retry when wanted) or at worst making the auto-upgrade to fail.
So I indeed do not really dare using the new upgrade mechanism for the next minor release for something as minor as a change of git branch. But this PR also contributes to making the new migration system more mature, e.g. fixing some white screens and logging issues.

@Alkarex Alkarex merged commit 97ba626 into FreshRSS:edge May 15, 2021
@Alkarex Alkarex deleted the git-update-auto-edge branch May 15, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants