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

Remove temp state because uc_upgrade now works on Studio without tempstate #1433

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Remove temp state because uc_upgrade now works on Studio without tempstate #1433

merged 1 commit into from
Oct 4, 2024

Conversation

brdandu
Copy link
Collaborator

@brdandu brdandu commented Sep 25, 2024

JIRA: ZAPP-1362

@paulr34
Copy link
Collaborator

paulr34 commented Sep 26, 2024

Ive been testing this and it appears to be working well. Leaning towards approving but am going to run through some more use cases to make sure.

paulr34
paulr34 approved these changes Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@paulr34 paulr34 left a comment

Choose a reason for hiding this comment

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

Approved

tecimovic
tecimovic reviewed Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@tecimovic tecimovic left a comment

Choose a reason for hiding this comment

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

Paul was not wrong we need to test this well, but in general it's a right thing.

@brdandu
Copy link
Collaborator Author

brdandu commented Sep 27, 2024 •
edited
Loading

Paul was not wrong we need to test this well, but in general it's a right thing.

I have tested this for matter and zigbee apps. Went through the matter application upgrade process with Michael Rupp from the matter team as well showing how it passes now. I totally agree with testing this and test any use cases someone mentions I should test but disagree with we should not remove this unless there is a concrete reason to do so.

paulr34
paulr34 requested changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@paulr34 paulr34 left a comment

Choose a reason for hiding this comment

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

I would like @tecimovic to approve before merge

@paulr34
Copy link
Collaborator

paulr34 commented Oct 2, 2024

@tecimovic @brdandu

we need testing for this change or else any change after is a risk, right?

tecimovic
tecimovic approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@tecimovic tecimovic left a comment

Choose a reason for hiding this comment

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

Right thing to do, the question is have we done enough testing. But I assume we did, so I'm ok with this.

@brdandu brdandu force-pushed the improvement/removeTempStateFromUcGenerate/ZAPP-1362 branch from c8b134d to f24eee3 Compare October 4, 2024 14:17
@brdandu brdandu merged commit 82e0353 into project-chip:master Oct 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanzhouyc ethanzhouyc ethanzhouyc approved these changes

@paulr34 paulr34 paulr34 approved these changes

@tecimovic tecimovic tecimovic approved these changes

@tbrkollar tbrkollar Awaiting requested review from tbrkollar

@dhchandw dhchandw Awaiting requested review from dhchandw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brdandu @paulr34 @tecimovic @ethanzhouyc