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

[JUJU-390] Add DisplayName to the Machine description #102

Merged
merged 2 commits into from Jan 25, 2022

Conversation

cderici
Copy link
Member

@cderici cderici commented Jan 22, 2022

This adds the DisplayName field for a machine to transfer its display name through the migration.

It is a pre-requisite for this PR on juju for fixing the LP #1954969.

QA steps

Added the display-name field in the unit tests within the cloudinstance_test.go, so the following should succeed.

  $ go test

@cderici cderici changed the title Add DisplayName to the Machine description [JUJU-390] Add DisplayName to the Machine description Jan 23, 2022
Copy link

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

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

LG2M

Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

We don't want to transport this with the machine document. It resides in the instanceData collection and should go against the cloudInstance type in this library.

Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Yes, this looks better.

@cderici
Copy link
Member Author

cderici commented Jan 25, 2022

$$merge$$

@jujubot jujubot merged commit cc5932c into juju:v3 Jan 25, 2022
jujubot added a commit to juju/juju that referenced this pull request Jan 28, 2022
#13645

#### Description

This allows the machines (usually provided by OCI or MAAS) to be able to transfer their `DisplayName` fields through a migration. 

Fixes:

 [LP #1954969](https://bugs.launchpad.net/juju/+bug/1954969).

Requires:

- [x] juju/description#102 to land.
- [x] #13653 to land.

#### QA Steps

You'll need either Oracle OCI or MAAS controllers for this one. (Other providers don't use display name) 
I tested it with the OCI, like as follows (I used the compartment&tenancy ids from within the cloud-city):

```sh
juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller2

juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller1

juju add-model --config compartment-id=$OCID_COMPARTMENT test-model1

juju deploy ubuntu
```

Note the machine `Inst Id` in juju status, or do `db.instanceData.find().pretty()` in mongodb and look for the `display-name` field.

```sh
juju migrate test-model1 oci-test-controller2
```

1. Migration should be successful,
2. After the migration is complete you should be able to see the same id in the juju status or the db.


#### Notes & Discussion

This PR also includes temporary code that will be removed before the PR lands:

- go.mod for external changed dependency (description pkg), and
- provider/oci/environ.go for quick fix to allow the PR to run
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants