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

fix: store state in deployment data #76

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

danielskinstad
Copy link
Collaborator

Removed the key and corresponding functions for storing the update_state
and the artifact type. It is now a part of the deployment data.

Changelog: Title
Ticket: MEN-7515

@danielskinstad danielskinstad requested review from larsewi, lluiscampos, vpodzime and pasinskim and removed request for larsewi September 19, 2024 08:32
core/src/mender-deployment-data.c Outdated Show resolved Hide resolved
core/src/mender-deployment-data.c Show resolved Hide resolved
include/mender-deployment-data.h Outdated Show resolved Hide resolved
core/src/mender-client.c Outdated Show resolved Hide resolved
@@ -518,7 +518,6 @@ mender_client_initialization_work_function(void) {

/* Delete pending deployment */
mender_storage_delete_deployment_data();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed should call the data update data, it's no longer just a deployment info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, you think I should rename everything that's named deployment data to update data (including mender-deployment-data.c etc.?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed should call the data update data (...)

To be fair with Daniel in the ticket we wrote "consider renaming..."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I but IIRC we discussed it on Slack or somewhere else too...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do it in a separate PR

core/src/mender-client.c Outdated Show resolved Hide resolved
@@ -1040,9 +1048,8 @@ mender_client_update_work_function(void) {
* verification should happen anyway, the callback in that
* state should be able to see if things went well or
* wrong. */
mender_storage_save_update_state(MENDER_UPDATE_STATE_VERIFY_REBOOT, mender_update_module->artifact_type);
mender_deployment_data_set_state(mender_client_deployment_data, MENDER_UPDATE_STATE_VERIFY_REBOOT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the point of setting the state here again?

Copy link
Collaborator Author

@danielskinstad danielskinstad Sep 19, 2024

Choose a reason for hiding this comment

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

According to the comment above it's to ensure that reboot verification happens in case of a crash in the reboot-callback, but it's also set at the start of the code, so I guess this is unnecessary?

core/src/mender-client.c Outdated Show resolved Hide resolved
include/mender-deployment-data.h Outdated Show resolved Hide resolved
core/src/mender-deployment-data.c Outdated Show resolved Hide resolved
core/src/mender-client.c Outdated Show resolved Hide resolved
core/src/mender-client.c Outdated Show resolved Hide resolved
core/src/mender-client.c Outdated Show resolved Hide resolved
core/src/mender-client.c Outdated Show resolved Hide resolved
include/mender-deployment-data.h Outdated Show resolved Hide resolved
include/mender-deployment-data.h Outdated Show resolved Hide resolved
include/mender-deployment-data.h Show resolved Hide resolved
Copy link
Collaborator

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

It looks good to me - it seems like all my concerns were addressed. Thanks!

include/mender-deployment-data.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

* verification should happen anyway, the callback in that
* state should be able to see if things went well or
* wrong. */
mender_storage_save_update_state(MENDER_UPDATE_STATE_VERIFY_REBOOT, mender_update_module->artifact_type);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we still want do do set_and_store_state() here iff there is a reboot callback. If there isn't, it will be called by NEXT_STATE below anyway. It just wouldn't be called twice in such a case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it outside of the if-statement, but I moved it back now, because it's stored in NEXT_STATE as you said

They are passed in as NULL and populated later.

Ticket: None

Signed-off-by: Daniel Skinstad Drabitzius <[email protected]>
Removed the key and corresponding functions for storing the update_state
and the artifact type. It is now a part of the deployment data.

Changelog: Title
Ticket: MEN-7515

Signed-off-by: Daniel Skinstad Drabitzius <[email protected]>
@mender-test-bot
Copy link

mender-test-bot commented Sep 24, 2024

Merging these commits will result in the following changelog entries:

Changelogs

mender-mcu (update-state)

New changes in mender-mcu since main:

Bug Fixes
  • store state in deployment data
    (MEN-7515)

@danielskinstad danielskinstad merged commit 6cfb689 into mendersoftware:main Sep 24, 2024
1 check passed
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.

5 participants