-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
CLOUDSTACK-9003 Make VirtualMachineName into an injectable service class #988
Conversation
This is one of few remaining classes that is not injectable. In most places that reference it, it can be injected easily. It should also be inejctable in the VMWare and HyperV resources which reference it since they are direct agents. Rationale: Updating the code to remove static spaghetti dependencies and paving the way for more internal extensibility.
This commit mmakes the VirtualMachineName class into a VirtualMachineNameService interface which has an implementation class loaded as a bean in the core-managers-context. This brings it in line with the dependency injection patterns used in the CloudStack management server.
I think I also plan on refactoring this pull request further to make the VirtualMachineNameService extensible similar to how some CloudStack adapters work, so the underlying generation of instance hypervisor guest names can be changed, depending on how someone running ACS wants the instance naming policies to work. |
Looks like build keeps crashing. Should I maybe wait until later to run it? |
not sure what this is @ProjectMoon . I am running a test against your PR overnight and will get back with what I may find, if anything. |
@ProjectMoon THe stuff built and is running tests. You should be able to force push it and it should build. unsure what we are looking for but probably a problem on the build slave. |
@@ -36,7 +36,7 @@ | |||
</list> | |||
</property> | |||
</bean> | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing space
@ProjectMoon impressive work. Can you create a jira ticket and describe it (including functional incentive) and prepend the PR with the ticket id? Also an integration test is in place I think. to validate the functionality. If you can find one that covers it in the existing ones, please mention it here. |
I can create a ticket, yes. I plan to push more work to this pull request today/tomorrow that moves the VirtualMachineName service and probably the UUID manager into its own module. This would make it very easy for 3rd party developers to override the default VM naming and UUID creation policies in CloudStack by excluding the module and providing their own in its place. |
@ProjectMoon Thanks for the PR. I cannot judge the actual change you made. I did run some generic tests and they seem to pass just fine:
Result:
The 3 errors at the bottom are due to CLOUDSTACK-8991 and unrelated to this PR. And:
Result:
|
While I haven't pushed new work to this PR yet, I do have a prototype of what I intend to do, and am looking for thoughts on it here. My plan:
|
@ProjectMoon : sounds good let me know of any more PoC code or further design. |
Closing this as we have finally finished our "real" implementation of this, though it's far beyond the scope of this PR. At some point in the near future we will submit a PR with a full refactoring of how resources are named. |
This pull request refactors the VirtualMachineName class into an injectable dependency. It is one of the few remaining classes that appears to be spaghetti tangled through many places in the code, and it hasn't been updated in years. These two commits introduce a VirtualMachineNameService which is injected everywhere it's relevant.
The rationale for this is to separate concerns better and remove spaghetti dependencies on static methods. It also opens up the door to easier extensibility in the future.
Some potential issues with this pull request: