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

lxd/init: Add support for storage volumes in preseed init #12426

Merged
merged 4 commits into from
Jan 18, 2024
Merged

lxd/init: Add support for storage volumes in preseed init #12426

merged 4 commits into from
Jan 18, 2024

Conversation

megheaiulian
Copy link
Contributor

@megheaiulian megheaiulian commented Oct 20, 2023 •
edited
Loading

Add support for configuring storage volumes in preseed init.

Fixes #10956

@github-actions github-actions bot added the API Changes to the REST API label Oct 20, 2023
@megheaiulian megheaiulian force-pushed the feat/preseed-storage-volumes branch 5 times, most recently from 8d13435 to 39563ea Compare October 24, 2023 04:13
tomponline
tomponline reviewed Oct 25, 2023
View reviewed changes
shared/api/init.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Oct 25, 2023
View reviewed changes
lxd/init.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Oct 25, 2023
View reviewed changes
shared/api/init.go Outdated Show resolved Hide resolved
tomponline
tomponline requested changes Oct 25, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks for this!

Please can you add some tests to test_init_preseed function

@github-actions github-actions bot added the Documentation Documentation needs updating label Oct 27, 2023
tomponline
tomponline reviewed Oct 27, 2023
View reviewed changes
lxd/init.go Outdated Show resolved Hide resolved
shared/api/init.go Outdated Show resolved Hide resolved
megheaiulian
megheaiulian commented Oct 27, 2023
View reviewed changes
lxd/init.go Outdated Show resolved Hide resolved
@megheaiulian
Copy link
Contributor Author

megheaiulian commented Oct 30, 2023 •
edited
Loading

@tomponline I think it should be ok now. Ran the tests also locally and they passed.
Can you please have another look at the code?

@megheaiulian megheaiulian force-pushed the feat/preseed-storage-volumes branch 2 times, most recently from eb6c1bc to 48c37f0 Compare October 31, 2023 18:04
@megheaiulian
Copy link
Contributor Author

Updated this again. The test was trying to delete the storage pool first and then the volume.

@megheaiulian megheaiulian force-pushed the feat/preseed-storage-volumes branch 2 times, most recently from 32e87b0 to 3be5c3d Compare November 14, 2023 06:38
tomponline
tomponline requested changes Dec 4, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

Please can you rebase this so that each commit makes up a single logical change, i.e we dont want to have multiple commits that make a change to, for example, lxd/init.go, and then later in the PR those initial changes are altered again.

@megheaiulian
Copy link
Contributor Author

megheaiulian commented Dec 4, 2023 •
edited
Loading

I've rebased this in 3 separate commits. Let me know if further changes are needed!

MusicDin
MusicDin reviewed Dec 4, 2023
View reviewed changes
shared/api/init.go Outdated Show resolved Hide resolved
@megheaiulian megheaiulian force-pushed the feat/preseed-storage-volumes branch 2 times, most recently from 8f4d1bc to bcfd396 Compare December 5, 2023 07:36
tomponline
tomponline reviewed Dec 5, 2023
View reviewed changes
shared/api/init.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Dec 5, 2023
View reviewed changes
doc/api-extensions.md Outdated Show resolved Hide resolved
@megheaiulian
Copy link
Contributor Author

@tomponline Is the test failure relevant? It is very hard to figure where exactly the tests have failed since the stack trace is huge?

@tomponline
Copy link
Member

You need to rebase your branch on upstream main to get the openfga revert changes

@megheaiulian megheaiulian force-pushed the feat/preseed-storage-volumes branch 3 times, most recently from b28170f to 67a31e7 Compare December 12, 2023 04:34
MusicDin
MusicDin previously approved these changes Dec 12, 2023
View reviewed changes
@tomponline
Copy link
Member

tomponline commented Jan 18, 2024 •
edited
Loading

Hi @megheaiulian please can you rebase this so the tests run again, we've recently changed the contributor requirements to require the Canonical CLA be signed for external contributors, see https://github.com/canonical/lxd/blob/main/CONTRIBUTING.md#license-and-copyright so we need the CLA check to re-run. Thanks

@megheaiulian
api: Add init_preseed_storage_volumes api extension
b3418ec 
Signed-off-by: Meghea Iulian <[email protected]>
@megheaiulian
lxd/init: Add support for storage volumes in preseed init
5b8adfd 
Add support for configuring storage volumes in preseed init.

Fixes canonical#10956

Signed-off-by: Meghea Iulian <[email protected]>
@megheaiulian
doc: Update rest api and init sample config
0a14441 
Signed-off-by: Meghea Iulian <[email protected]>
@megheaiulian
Copy link
Contributor Author

I've signed the CLA. Please retry the checks.

tomponline
tomponline approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@tomponline tomponline merged commit 9ee243a into canonical:main Jan 18, 2024
26 checks passed
@megheaiulian megheaiulian deleted the feat/preseed-storage-volumes branch January 18, 2024 14:51
@megheaiulian megheaiulian mentioned this pull request Feb 20, 2024
Open
@ru-fu ru-fu mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@MusicDin MusicDin MusicDin left review comments

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for configuring volumes to the preseed file
3 participants
@megheaiulian @tomponline @MusicDin