Skip to content

Storage system_storage directory restructure #13314

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
Merged
merged 2 commits into from
Jul 27, 2020
Merged

Storage system_storage directory restructure #13314

merged 2 commits into from
Jul 27, 2020

Conversation

rajkan01
Copy link
Contributor

@rajkan01 rajkan01 commented Jul 20, 2020 •
edited
Loading

Summary of changes

Preceding PR: #13300 #13307

  • Restructured storage/system_storage as per the new directory structure proposal:
storage
├── blockdevice
├── filesystem
├── kvstore
└── platform // Renamed from system_storage for consistency with other folders
    ├── mbed_lib.json
    └── source
        └── PlatformStorage.cpp // renamed from _**SystemStorage.cpp**_
  • Renamed directory name system_storage => platform, source SystemStorage.cpp => PlatformStorage.cpp and retained system-storage as config library name in mbed_lib.json to avoid breaking any applicaiton.

Impact of changes

None.

Migration actions required

None.

Documentation

To Be Updated.

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual testing: (Build for K64F target with GCC_ARM toolchain)

  • mbed-os-example-blockdevice
  • mbed-os-example-filesystem
  • mbed-os-example-kvstore
  • Greentea tests: Full and Bare metal profiles
  • Unit tests build and run

Reviewers

@0xc0170 @ARMmbed/mbed-os-core

@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jul 20, 2020
@ciarmcom ciarmcom requested review from 0xc0170 and a team July 20, 2020 13:00
@ciarmcom
Copy link
Member

@rajkan01, thank you for your changes.
@0xc0170 @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

evedon
evedon approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

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

Should be fine.
In the impact of change part of the description, add that "system-storage" library is renamed "platform-storage". I don't think it will have affect applications but it should be mentioned.

@mergify mergify bot added needs: CI and removed needs: review labels Jul 20, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

In the impact of change part of the description, add that "system-storage" library is renamed "platform-storage". I don't think it will have affect applications but it should be mentioned.

I believe this would be good to have. I would rather mark all as feature changes and describe them in the release notes. or at least to add one section to the release notes: a table with old and new location so if it affects anyone, would migrate easily or just find out where suddenly the code was moved.

Something like this would make sense?

Old location: New location:
features/something/old_lication connectivity/something

0xc0170
0xc0170 requested changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Let's review proposal for "Migration" documenation (I approved the changes but not documentation as it is not yet closed). I'll start CI

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 22, 2020

CI started

@mergify mergify bot added needs: work and removed needs: CI labels Jul 22, 2020
@mbed-ci
Copy link

mbed-ci commented Jul 22, 2020

Test run: FAILED

Summary: 2 of 3 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@evedon
Copy link
Contributor

evedon commented Jul 23, 2020

Should be fine.
In the impact of change part of the description, add that "system-storage" library is renamed "platform-storage". I don't think it will have affect applications but it should be mentioned.

This is actually used by the bootloader https://github.com/ARMmbed/mbed-bootloader/blob/master/mbed_app.json/#L6 so we should revert to naming the library "platform-storage".

@mergify
Copy link

mergify bot commented Jul 23, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@rajkan01 rajkan01 force-pushed the storage_sys_dir_restructure branch 2 times, most recently from 475a9e0 to 648eed1 Compare July 23, 2020 15:21
@rajkan01 rajkan01 force-pushed the storage_sys_dir_restructure branch from 648eed1 to 59072e9 Compare July 23, 2020 15:32
@rajkan01
Copy link
Contributor Author

Should be fine.
In the impact of change part of the description, add that "system-storage" library is renamed "platform-storage". I don't think it will have affect applications but it should be mentioned.

This is actually used by the bootloader https://github.com/ARMmbed/mbed-bootloader/blob/master/mbed_app.json/#L6 so we should revert to naming the library "platform-storage".

Yes, I retained "system-storage" as config library name in mbed_lib.json to avoid breaking any application.

@rajkan01
Copy link
Contributor Author

In the impact of change part of the description, add that "system-storage" library is renamed "platform-storage". I don't think it will have affect applications but it should be mentioned.

I believe this would be good to have. I would rather mark all as feature changes and describe them in the release notes. or at least to add one section to the release notes: a table with old and new location so if it affects anyone, would migrate easily or just find out where suddenly the code was moved.
Something like this would make sense?

Old location:
New location:

features/something/old_lication
connectivity/something

@0xc0170 We retained "system-storage" as config library name in mbed_lib.json to avoid breaking

@rajkan01
Copy link
Contributor Author

@0xc0170 I have resolved previously reported CI issue and rebased aginst master. Could you restart the CI please

@rajkan01
Copy link
Contributor Author

Let's review proposal for "Migration" documenation (I approved the changes but not documentation as it is not yet closed). I'll start CI

@0xc0170 There is no "Migration" documentation required for this PR changes

@0xc0170 0xc0170 self-requested a review July 24, 2020 09:39
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 24, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Jul 24, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

@rajkan01
Copy link
Contributor Author

@0xc0170 This PR CI passed, could you merge

@0xc0170 0xc0170 merged commit 315b6bd into ARMmbed:master Jul 27, 2020
@mergify mergify bot removed the ready for merge label Jul 27, 2020
@mbedmain mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@evedon evedon evedon approved these changes

Assignees
No one assigned
Labels
release-version: 6.2.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rajkan01 @ciarmcom @0xc0170 @mbed-ci @evedon @mbedmain