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

Append to existing apps.json preload state instead of overwriting #242

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alexgg
Copy link
Contributor

@alexgg alexgg commented May 7, 2021

This is to remain in draft state until the v3 target state API changes are merged


If the image we are preloading to contains an existing apps.json append
the preloaded app to it instead of overwriting it.

Fixes balena-io/balena-cli#2262

Relates to balena-io/balena-cli#2263

Change-type: patch
Signed-off-by: Alex Gonzalez [email protected]


Tested on a raspberrypi4-64-2.75.0+rev1 image that does not contain a preloaded apps.json, and on a genericx86-64-ext development build with an existing apps.json with preloaded state.

@alexgg alexgg marked this pull request as draft May 7, 2021 12:20
@alexgg alexgg requested review from klutchell and pdcastro May 7, 2021 12:30
@alexgg alexgg marked this pull request as ready for review May 7, 2021 12:30
@alexgg alexgg marked this pull request as draft May 7, 2021 12:49
src/preload.py Outdated Show resolved Hide resolved
@alexgg alexgg force-pushed the 2262-alexgg-preload branch from 8ed207a to 08bff28 Compare May 7, 2021 15:50
src/preload.py Outdated Show resolved Hide resolved
src/preload.py Outdated Show resolved Hide resolved
src/preload.py Outdated Show resolved Hide resolved
@alexgg alexgg force-pushed the 2262-alexgg-preload branch 2 times, most recently from 681e5c7 to d068f5d Compare May 24, 2021 16:57
src/preload.py Outdated Show resolved Hide resolved
src/preload.py Outdated Show resolved Hide resolved
@alexgg alexgg force-pushed the 2262-alexgg-preload branch 3 times, most recently from e3a74c6 to 1575c86 Compare May 25, 2021 11:30
src/preload.py Outdated Show resolved Hide resolved
src/preload.py Outdated Show resolved Hide resolved
@alexgg alexgg force-pushed the 2262-alexgg-preload branch 2 times, most recently from 57b64ff to 8e1ab6d Compare May 26, 2021 07:54
src/preload.py Outdated Show resolved Hide resolved
src/preload.py Outdated Show resolved Hide resolved
@alexgg alexgg force-pushed the 2262-alexgg-preload branch 2 times, most recently from 5e4a78c to 4f6b4d6 Compare June 2, 2021 18:40
@alexgg
Copy link
Contributor Author

alexgg commented Jun 2, 2021

Validation on a devenv with a draft v3 state endpoint:

[x] Preloading on an image with preloaded apps.json with a v3 state supervisor
* v3 style app is appended to apps.json
[x] Preloading on an image with preloaded apps.json with a v2 state supervisor (should not happen)
Fails with:

The preloaded app state and the current state in the image have a version mismatch. Please use afresh, not previously preloaded image file

[x] Preloading on an image without apps.json with a v2 state supervisor
* v2 stype app appears in apps.json
[x] Preloading on an image without apps.json with a v3 state supervisor (should not happen)

  • v3 state app appears in apps.json

@alexgg alexgg force-pushed the 2262-alexgg-preload branch 2 times, most recently from 87a7189 to 85e0fb0 Compare June 4, 2021 08:39
@alexgg alexgg requested a review from pdcastro June 4, 2021 08:53
@alexgg alexgg force-pushed the 2262-alexgg-preload branch 2 times, most recently from 715f3e9 to fc0dc06 Compare June 4, 2021 09:00
Update the supervisor repository regex so the supervisor version can be
correctly extracted from the images.

Change-type: patch
Signed-off-by: Alex Gonzalez <[email protected]>
alexgg added 2 commits June 7, 2021 11:04
If the image we are preloading to contains an existing `apps.json` append
the preloaded app to it instead of overwriting it.

Fixes balena-io/balena-cli#2262

Change-type: patch
Signed-off-by: Alex Gonzalez <[email protected]>
Newer supervisors support v3 target state that support multiple apps in
the target state and use a uuid as index.

Fixes #245

Change-type: patch
Signed-off-by: Alex Gonzalez <[email protected]>
@alexgg alexgg force-pushed the 2262-alexgg-preload branch from fc0dc06 to e9a611b Compare June 7, 2021 09:05
Copy link
Contributor

@pdcastro pdcastro left a comment

Choose a reason for hiding this comment

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

Small things while it is still a draft -- I'd have approved the PR if it wasn't a draft. :-)

@@ -739,6 +739,15 @@ class Preloader extends EventEmitter {
}
}

_supervisorLT13() {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid logic duplication, _supervisorLT7 could be renamed _supervisorLT and take the version as a parameter. To preserve the existing "typo safety feature", the version strings could be assigned to module-level constants. For example:

const v7 = '7.0.0';
const v13 = '13.0.0';

class Preloader extends EventEmitter {
	...
	_supervisorLT(version) {
		try {
			return compareVersions(this.supervisorVersion, version) === -1;
		...
	}
  • this._supervisorLT7()this._supervisorLT(v7)
  • this._supervisorLT13()this._supervisorLT(v13)

Comment on lines +619 to +620
for service in app['services']:
image_name = app['services'][service]['image']
Copy link
Contributor

Choose a reason for hiding this comment

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

The image property is guaranteed to exist, right? Otherwise, .get() could be used instead of the bracket syntax to avoid a KeyError, plus providing the empty string as a default value:

Suggested change
for service in app['services']:
image_name = app['services'][service]['image']
for service in app['services'].values():
image_name = service.get('image', '')

get_images_and_supervisor_version()
)

for uuid in list(current_state['apps'].keys()):
Copy link
Contributor

Choose a reason for hiding this comment

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

A line of code can be deleted (app = current_state['apps'][uuid]) later on by using items() instead of keys():

Suggested change
for uuid in list(current_state['apps'].keys()):
for [uuid, app] in current_state['apps'].items():

In Python 3 (the version used by preload), keys() values() items() return iterators and list() can be used to create an indexable list/array out of the iterators where needed, but it doesn't look needed here.

raise Exception("State version mismatch")

# Halt if user apps are present in current state
app = current_state['apps'][uuid]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be deleted if the [uuid, app] unpacking suggested in another comment is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the apps preloading to merge to an existing apps.json file if it exists
2 participants