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

feat: add migration import check for charms without manifest #18141

Merged
merged 1 commit into from
Sep 26, 2024
Merged

feat: add migration import check for charms without manifest #18141

merged 1 commit into from
Sep 26, 2024

Conversation

Aflynn50
Copy link
Contributor

@Aflynn50 Aflynn50 commented Sep 25, 2024 •
edited
Loading

Charms without a manifest.yaml are not supported in 4.0. This check blocks migration of models that contain such charms.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing

QA steps

Migration of 3.6 - 4.0
sj here is stands for snap juju, which is set to 3.6/edge.
'juju' is the compiled 4.0 from this PR.

# Checkout 3.6 to use the version of the ubuntu-plus charm without a manifest.
$ git checkout 3.6
$ sj bootstrap lxd source
$ juju bootstrap lxd target
$ juju switch migraiton-source
$ sj add-model test-mig
$ sj deploy ./testcharms/charms/ubuntu-plus
$ sj migrate test-mig target
ERROR target prechecks failed: migration import prechecks: checking model for charms without manifest.yaml: all charms now require a manifest.yaml file, this model hosts charm(s) with no manifest.yaml file: ubuntu-plus

Check happy path migration:

$ sj remove-application ubuntu-plus
$ sj deploy ubuntu
$ sj migrate test-mig target
Migration started with ID "6c6fbb18-299b-4dd0-8edc-785088e2b296:0"
$ juju switch target:test-mig
$ juju status
Model     Controller  Cloud/Region  Version      SLA          Timestamp
test-mig  source      lxd/default   3.6-beta3.1  unsupported  11:07:29+01:00

App     Version  Status  Scale  Charm   Channel        Rev  Exposed  Message
ubuntu  22.04    active      1  ubuntu  latest/stable   24  no       

Unit       Workload  Agent  Machine  Public address  Ports  Message
ubuntu/0*  active    idle   1        10.101.18.172          

Machine  State    Address        Inst id        Base          AZ  Message
1        started  10.101.18.172  juju-e2b296-1  ubuntu@22.04      Running

Links

Launchpad bug: https://bugs.launchpad.net/juju/+bug/2080359

Jira card: JUJU-6727

@Aflynn50 Aflynn50 mentioned this pull request Sep 25, 2024
Closed
4 tasks
SimonRichardson
SimonRichardson reviewed Sep 25, 2024
View reviewed changes
Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

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

A lot, lot nicer!

Just going to run the QA steps, but very happy with this addition.

internal/migration/precheck.go Outdated

err := checkForCharmsWithNoManifest(model)
if err != nil {
return fmt.Errorf("checking model for charms without manifest.yaml: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Use the internal errors package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

internal/migration/precheck.go
@@ -71,6 +74,20 @@ func SourcePrecheck(
return nil
}

// ImportPrecheck checks the data being imported to make sure preconditions for
// importing are met.
func ImportPrecheck(
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if we should revisit how all this is plumbed together. For now, this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like it would be well worth doing. It would be good to at least move the migration checks that are currently in the upgradevalidation package to here, i.e. move them from the source controller to the target controller. A lot of them use state but it should be possible to just look at the actual model description.

@Aflynn50
feat: add migration import check for charms without manifest
3b6fc99 
Charms without a manifest.yaml are not supported in 4.0. This check
blocks migration of models that contain such charms.
gfouillet
gfouillet requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@gfouillet gfouillet left a comment

Choose a reason for hiding this comment

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

LGTM but TU are broken, and it may be linked to the fix. I think they need to be updated.

@hpidcock hpidcock added the 4.0 label Sep 25, 2024
@Aflynn50
Copy link
Contributor Author

/build

@Aflynn50
Copy link
Contributor Author

LGTM but TU are broken, and it may be linked to the fix. I think they need to be updated.

Looks like random failures tbh, I've just triggered a build, hopefully that gets it. The failing test passed locally and looked a bit racy.

@Aflynn50
Copy link
Contributor Author

/merge

@jujubot jujubot merged commit f2c5cdc into juju:main Sep 26, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonRichardson SimonRichardson SimonRichardson approved these changes

@gfouillet gfouillet gfouillet approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@Aflynn50 @SimonRichardson @gfouillet @hpidcock @jujubot