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

Fluent flow in VM::OSDisk #697

Closed
anuchandy opened this issue May 10, 2016 · 11 comments
Closed

Fluent flow in VM::OSDisk #697

anuchandy opened this issue May 10, 2016 · 11 comments

Comments

@anuchandy
Copy link
Member

anuchandy commented May 10, 2016

This is a thread for discussing design of withImage, defineOSDisk variants and withExistingOSDisk that needs to be enabled in VM fluent definition flow.

The withImage variants allows basic user to easily declare the image to be used for the VM's OS Disk (with default configuration)

The defineOSDisk variants allows user to declare the image along with OS disk configuration, such as caching, size, encryption settings etc..

The withExistingOSDisk allows user to attach an existing OS Disk VHD for the VM.

office lens 20160510-144554

withImage(ImageReference imageReference);
withLatestImage(String publisher, String offer, String sku);
withKnownImage(KnownVirtualMachineImage knownImage);
withUserImage(String containerName, String vhdName);

fromImage(ImageReference imageReference);
fromLatestImage(String publisher, String offer, String sku);
fromKnownImage(KnownVirtualMachineImage knownImage);
fromUserImage(String containerName, String vhdName);

withExistingOSDisk(String containerName, String vhdName, OperatingSystemTypes osType);

defineOSDisk(String name);
storeVhdAt(String containerName, String vhdName);
DefinitionWithDataDisk attach();
@jianghaolu
Copy link
Contributor

jianghaolu commented May 13, 2016

Updated.
grammar

@anuchandy
Copy link
Member Author

@jianghaolu can you put the EBNF notation for the same?

@jianghaolu
Copy link
Contributor

Grammar ::= ( 
  ('withNewStorageAccount' | 'withExistingStorageAccount')
  (('withImage(reference)' | 'withImage(url)' | 'withLatestImage' | 'withKnownImage') 
   ('withLinuxOS' 'withRootUsername' 
     (('withSsh(key)' | 'withSsh(list)') ('withPassword' | 'withoutPassword') | 
     'withoutSsh' 'withPassword') | 
    'withWindowsOS'
      (('defineConfiguration' 'enableVMAgent'? 'enableAutoUpdate'? 'withTimeZone'? 'withWinRM'? 'apply')?
      'withAdminUserName' 'withPassword'))
  ) | ('withWindowsImage' | 'withLinuxImage'))

@anuchandy
Copy link
Member Author

Thanks Jianghaolu.

For others who are reading this discussion: Above is Extended Backus-Naur Form (EBNF) notation representing the context and method chains in fluent API for VM OSDisk and OSProfile. The Railroad diagram is generated using http://www.bottlecaps.de/rr/ui.

@jianghaolu
Copy link
Contributor

jianghaolu commented May 13, 2016

From the graph, it may make more sense to move the admin/password setup for Windows before the configurations as they are the required parameters.

@anuchandy
Copy link
Member Author

Yes, in general we follow the pattern of keep "optionals parameters" after all "required parameters".

But in this case - was thinking that "configuration" is specific to "Windows OS" [autoupdate, winrm] and keeping it close to "WithWindowsOS()" provide better redability than placing it after "withAdminPassword". "withAdminPassword" is applicable for both Windows and Linux.

Do you have any specific reason for this suggestion other than the problem of breaking the pattern?

azure.virtualmachines().define("vm-name")
     .withRegion("region")
     .withNewResourceGroup("rg1")
     .withImage("WinServer", , ,)
     .withWindowsOS()
     .defineConfiguration()
        .disableVMAgent()
        .withWinRMListeners(lst)
        .apply()
     .withAdminUserName(uname)
     .withAdminPassword(pwd)
     .withNetwork("10.1.1.1/24")
     .withPublicIP(dns-name)
     .create();

v/s

azure.virtualmachines().define("vm-name")
     .withRegion("region")
     .withNewResourceGroup("rg1")
     .withImage("WinServer", , ,)
     .withWindowsOS()
     .withAdminUserName(uname)
     .withAdminPassword(pwd)
     .defineConfiguration()
        .disableVMAgent()
        .withWinRMListeners(lst)
        .apply()
     .withNetwork("10.1.1.1/24")
     .withPublicIP(dns-name)
     .create();

Also, @selvasingh recommended to use "set()" instead of "apply()", that make sense because we are already using "apply()" in "Updatble" chain.

Instead of "disableVMAgent", we can use "withoutVMAgent" - The name "disableVMAgent()" gives an impression that - VM agent is installed by default but user want to disable it. In reality, if this flag is false Azure won't install VM Agent.

@jianghaolu
Copy link
Contributor

I don't feel strongly towards either of the options. But I personally like apply() as it's the word showing on the buttons in Windows setting dialogs for 20 years :)
We can change existing .apply() to .update() in light of the move from .provision() to .create().

@anuchandy
Copy link
Member Author

anuchandy commented May 14, 2016

Ok, we can decide on placement of config options later.

Agree, so for top-level resources renames are provision() -> create(), apply() -> update()

For optional child complex definitions that start with 'defineXXXXX()' can be end with apply() OR set() (e.g. defineConfiguration in the above diagram)

Below is the diagram showing different approach we discussed to set the intermediate complex optionals.

grammar

Grammar ::= ( 
  ('withNewStorageAccount' | 'withExistingStorageAccount')
  (('withImage(reference)' | 'withImage(url)' | 'withLatestImage' | 'withKnownImage') 
   ('withLinuxOS' 'withRootUsername' 
     (('withSsh(key)' | 'withSsh(list)') ('withPassword' | 'withoutPassword') | 
     'withoutSsh' 'withPassword') | 
    'withWindowsOS'
      (( 'withoutVMAgent' | 'disableAutoUpdate' | 'withWinRM' | 'withTimeZone' )* 
      'withAdminUserName' 'withPassword'))
  ) | ('withWindowsImage' | 'withLinuxImage'))

@anuchandy
Copy link
Member Author

anuchandy commented May 16, 2016

Another way of doing this is, move admin settings for Windows before the optional configurations as below:

grammar

Grammar ::= ( 
  ('withNewStorageAccount' | 'withExistingStorageAccount')
  (('withImage(reference)' | 'withImage(url)' | 'withLatestImage' | 'withKnownImage') 
   ('withLinuxOS' 'withRootUsername' 
     ((('withSsh(key)' | 'withSsh(list)') ('withPassword' | 'withoutPassword') | 
     'withoutSsh' 'withPassword')) | 
    'withWindowsOS'
      ('withAdminUserName' 'withPassword') ( 'withoutVMAgent' | 'disableAutoUpdate' | 'withWinRM' | 'withTimeZone' )* ) 
  ) | ('withWindowsImage' | 'withLinuxImage')) 'withYYYYNetwork(...)'

@anuchandy
Copy link
Member Author

anuchandy commented May 16, 2016

_DefinitionCreatable_

definitioncreatable

DefinitionCreatable
         ::= ( 'withPassword()' | 'withNewStorageAccount(name)' | 'withNewStorageAccount(StorageCreatable)' | 'withExistingStorageAccount(name)' )* 'create()'

_DefinitionLinuxCreatable_

definitionlinuxcreatable

DefinitionLinuxCreatable
         ::= 'withSSH(key)'* DefinitionCreatable

_DefinitionWindowsCreatable_

definitionwindowscreatable

DefinitionWindowsCreatable
         ::= ( 'withTimeZone(zone)' | 'withoutVMAgent()' | 'disableAutoUpdate()' | 'withWinRM(listener)' )* DefinitionCreatable

_DefinitionWithMarketPlaceImage_

definitionwithmarketplaceimage

DefinitionWithMarketPlaceImage
         ::= 'withMarketPlaceImage()' ( 'popular(imageEnum)' | 'latest(p, o, s)' | 'version(p, o, s, v)' )

_DefinitionVMCreatable_

definitionvmcreatable

DefinitionVMCreatable
         ::= ( DefinitionWithMarketPlaceImage | 'withStoredImage(url)' ) ( 'withWindows()' 'withAdminUserName(uname)' DefinitionWindowsCreatable | 'withLinux()' 'withRootUserName(uname)' DefinitionLinuxCreatable )
           | 'withOSDisk(url, osType)' DefinitionCreatable

@jianghaolu
Copy link
Contributor

Beta2 released.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants