Merge ~msaikia/cloud-init:topic-msaikia-vmware into cloud-init:master
| Status: | Merged |
|---|---|
| Approved by: | Chad Smith on 2017-08-15 |
| Approved revision: | d9e02b9b1d777445c2a8151f56fa3a1a3ba35e1a |
| Merge reported by: | Chad Smith |
| Merged at revision: | d9e02b9b1d777445c2a8151f56fa3a1a3ba35e1a |
| Proposed branch: | ~msaikia/cloud-init:topic-msaikia-vmware |
| Merge into: | cloud-init:master |
| Diff against target: |
280 lines (+180/-6) 4 files modified
cloudinit/sources/DataSourceOVF.py (+62/-1) cloudinit/sources/helpers/vmware/imc/config.py (+21/-3) cloudinit/sources/helpers/vmware/imc/config_passwd.py (+67/-0) tests/unittests/test_vmware_config_file.py (+30/-2) |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Chad Smith | Approve on 2017-08-15 | ||
| Server Team CI bot | continuous-integration | 2017-04-21 | Approve on 2017-08-09 |
| Scott Moser | 2017-04-21 | Pending | |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2016-09-12.
Commit Message
vcloud directoy: Guest Customization support for passwords
This feature enables the following VMware VCloud Director functionalities:
1. Setting admin password
2. Expire password.
3. Set admin password and expire.
Password configuration is triggered only as part of a full recustomization, that happens either on first power on or when "poweron and full recustomization" is selected. Full customization flow is determined by marker files. Unique marker ids are generated when full recustomization is requested. And marker file based on these marker ids help to determine if we need to execute the above configuration.
Description of the Change
1. Retrieve values from configuration file.
2. Parse data and call password customization.
3. Unit tests for retrieved password data
| Maitreyee Saikia (msaikia) wrote : | # |
| Maitreyee Saikia (msaikia) wrote : | # |
Updated some formatting.
| Scott Moser (smoser) wrote : | # |
Please set a combined commit message ('Set commit message' above.
Can you explain where you're going also?
I'm concerned about adding more "vmware" paths for configuring things that are done elsewhere (or not elsewhere) in cloud-init. We'd like have consistent paths for doing things as much as possible.
I understand that you're trying to have cloud-init take over another more solution, and I'm not entirely opposed to that, but I want to integrate as much as possible rather than having specific paths and function on vmware.
| Maitreyee Saikia (msaikia) wrote : | # |
> Please set a combined commit message ('Set commit message' above.
>
> Can you explain where you're going also?
> I'm concerned about adding more "vmware" paths for configuring things that are
> done elsewhere (or not elsewhere) in cloud-init. We'd like have consistent
> paths for doing things as much as possible.
>
> I understand that you're trying to have cloud-init take over another more
> solution, and I'm not entirely opposed to that, but I want to integrate as
> much as possible rather than having specific paths and function on vmware.
Thanks for your input Scott. I have updated the commit message.
This changeset is just to retrieve data from customization spec, for implementing some other customization functionalities like setting password, running post customization scripts etc. I shall post another changeset with the password configurator, which manipulates the values retrieved from these methods to be used by cloud-init's cc_set_password. We will be using as much cloud-init code as possible. For other functionalities like running pre and post customization specs that will be uploaded by the user, I dont think there is existing support. Please let me know if that sounds ok. Also for functions that are "vmware" specific, shall we still keep it in the helper path that we have created for vmware, or do we want to move them to some common code area?
Thanks
Maitreyee
FAILED: Continuous integration, rev:312808680eb
https:/
Executed test runs:
SUCCESS: https:/
FAILURE: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:7c480f71a20
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Scott Moser (smoser) wrote : | # |
There is one small nitpick in line.
There isn't anything wrong with this merge proposal, but it
a.) doesn't really do anything
I realize you're planning on using these things going forward, and thats fine, but there is no reason to accept this MP until we also see that further work. So just add it here. You can rebase/squahs your commits into single function items in a "series" if that helps you.
b.) should add some unit tests.
unit tests will help stop others from breaking your code path inadvertently.
| Scott Moser (smoser) wrote : | # |
Hi. I'm going to put this in "work in progress".
See the comments above. When youv'e addressed them, move it back to Needs Review.
Thanks
Scott
PASSED: Continuous integration, rev:c886370ebec
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Maitreyee Saikia (msaikia) wrote : | # |
Hi Scott
I have resubmitted my review request after addressing your comments.
https:/
Please let me know if you get a chance to review the new changeset.
Thanks
Maitreyee
-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Scott Moser
Sent: Tuesday, March 14, 2017 7:25 AM
To: Maitreyee Saikia <email address hidden>
Subject: Re: [Merge] ~msaikia/
Hi. I'm going to put this in "work in progress".
See the comments above. When youv'e addressed them, move it back to Needs Review.
Thanks
Scott
--
https:/
You are the owner of ~msaikia/
| Scott Moser (smoser) wrote : | # |
Your commit message is probably out of date. Update that please.
Missing from it is at very least a mention of 'markerfile'. I have a feeling you're doing something that might be already done in util.FileSemaph
Thats just a quick review.
There are probably some other things.
Thanks!
Scott
- a884268... by Maitreyee Saikia on 2017-06-02
PASSED: Continuous integration, rev:324eb7e3ad0
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Maitreyee Saikia (msaikia) wrote : | # |
> Your commit message is probably out of date. Update that please.
> Missing from it is at very least a mention of 'markerfile'. I have a feeling
> you're doing something that might be already done in util.FileSemaph
>
> Thats just a quick review.
> There are probably some other things.
>
> Thanks!
> Scott
Thanks Scott for the reviews. I have updated the changeset and commit message with your comments. As for the Markerfile, it does not have anything to do with semaphores. Here is an explanation of what our marker file is used for. vCloud Director creates a new marker id every time a full customization is requested. And since a new marker id is generated and we do not have the corresponding marker file as yet, we go forward and execute password configurator and then create the marker file in order that this code execution doesnot happen in future unless a new marker id is generated (new marker id is generated only on first power on or when "power on and force recustomization" is selected). So, I do not feel we can use FileSemaphores here.
| Scott Moser (smoser) wrote : | # |
i think i'm fine with this. one tiny comment in line.
- a080ca6... by Maitreyee Saikia on 2017-08-03
| Maitreyee Saikia (msaikia) wrote : | # |
> i think i'm fine with this. one tiny comment in line.
Addressed. Thanks.
FAILED: Continuous integration, rev:db962f2e31d
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:f9a1fa4f56c
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
| Ryan Harper (raharper) wrote : | # |
I believe you need to rebase to master to resolve this issue. You'll likely need to push --force your branch here to overwrite the history because of the rebase.
- 0531d33... by Maitreyee Saikia on 2017-08-07
- 2395779... by Maitreyee Saikia on 2017-08-07
- 3da515e... by Maitreyee Saikia on 2017-08-07
FAILED: Continuous integration, rev:3da515eeaaf
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
FAILED: CentOS 6 & 7: Build & Test
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
Thanks for the submit and pinging us. Here's a brief review of your code. Most and nits, feel free to object if you feel strongly one way or another. Again thanks for your time on this. I'll watch for updates to see where we get.
- 4cec8b6... by Maitreyee Saikia on 2017-08-08
FAILED: Continuous integration, rev:4cec8b64ce6
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
FAILED: CentOS 6 & 7: Build & Test
Click here to trigger a rebuild:
https:/
- d9e02b9... by Maitreyee Saikia on 2017-08-09
PASSED: Continuous integration, rev:d9e02b9b1d7
https:/
Executed test runs:
SUCCESS: Checkout
SUCCESS: Unit & Style Tests
SUCCESS: Ubuntu LTS: Build
SUCCESS: Ubuntu LTS: Integration
SUCCESS: CentOS 6 & 7: Build & Test
IN_PROGRESS: Declarative: Post Actions
Click here to trigger a rebuild:
https:/
| Chad Smith (chad.smith) wrote : | # |
I'm +1 with this changeset, thanks for the good work.


Thanks for the comments Joshua.
Updated the diff.