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

Require migration.stateful for containers too #781

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

Abhiram824
Copy link
Contributor

No description provided.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Apr 23, 2024
@stgraber
Copy link
Member

API extensions should always be appended to the existing list, not added at the top or middle of the files.

internal/instance/config.go Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
internal/server/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
@stgraber
Copy link
Member

I left a few more comments in line. General structure looks good so it's just about fixing a bunch of small things.

Also make sure to run both gofmt -s -w ./ and make static-analysis to fix some formatting issues and then make sure that there's nothing else that needs fixing.

@Abhiram824
Copy link
Contributor Author

I left a few more comments in line. General structure looks good so it's just about fixing a bunch of small things.

Also make sure to run both gofmt -s -w ./ and make static-analysis to fix some formatting issues and then make sure that there's nothing else that needs fixing.

I ran both commands. make static-analysis was able to run for everything except for rest-api-up-to-date, but since there were no changes to doc/rest-api.yaml I just moved on for now

@stgraber
Copy link
Member

I ran both commands. make static-analysis was able to run for everything except for rest-api-up-to-date, but since there were no changes to doc/rest-api.yaml I just moved on for now

It's telling you that there should be a change to doc/rest-api.yaml.

Run make update-api, that will generate the applicable change to doc/rest-api.yaml, then add that in a new commit title: doc/rest-api: Refresh swagger YAML

@stgraber
Copy link
Member

Looking into that one now.

@stgraber
Copy link
Member

Changes I made:

  • Rebased on current main branch
  • Moved changes to generated files into their own commit
  • Rebased the fix commits into the main commits
  • Slightly reworded the error messages
  • Added bug reference to commit

Now just waiting to see if Github Actions catch anything else :)

@stgraber
Copy link
Member

Also had to move the API extension to the bottom of the file.

Abhiram824 and others added 4 commits April 25, 2024 07:33
Signed-off-by: Abhiram824 <abhisuhaas1@gmail.com>
Signed-off-by: Abhiram824 <abhisuhaas1@gmail.com>
Closes lxc#682

Signed-off-by: Abhiram824 <abhisuhaas1@gmail.com>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
@stgraber stgraber marked this pull request as ready for review April 25, 2024 06:34
@stgraber stgraber changed the title Migration stateful feature added (WIP) have not tested yet Require migration.stateful for containers too Apr 25, 2024
@stgraber stgraber merged commit 9d5db85 into lxc:main Apr 25, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

None yet

2 participants