search

riptano / ComboAMI

The AMI takes a set of input parameters via the EC2 user-data to install, RAID, ring, and launch a DataStax Enterprise/Community cluster.
69 stars 59 forks source link

Allow customizing repository used for AMI configuration #74

Closed elad closed 9 years ago

elad commented 9 years ago

This is a reference pull request related to the issues raised in #72, specifically allowing a custom repository (instead of the one used to create the AMI) and allowing more than one hard-coded key for commit verification.

elad commented 9 years ago

Hey, just pushed a bunch of changes that I think address the points raised in our discussion. I'm using git rev-parse to detect if the second half of the argument to --repository is a commit id or not. In the latter case I prefix it with the remote name (origin) so it doesn't bubble to the user interface. Backwards compatibility is maintained and if --forcecommit is specified it takes precedence over everything else and forces use of the official repository.

I hope to test it later today.

mlococo commented 9 years ago

Awesome, I didn't even know rev-parse existed. I'll have a gander to review later today.

On Tue, Apr 14, 2015 at 8:55 AM, Elad Efrat notifications@github.com wrote:

Hey, just pushed a bunch of changes that I think address the points raised in our discussion. I'm using git rev-parse to detect if the second half of the argument to --repository is a commit id or not. In the latter case I prefix it with the remote name (origin) so it doesn't bubble to the user interface. Backwards compatibility is maintained and if --forcecommit is specified it takes precedence over everything else and forces use of the official repository.

I hope to test it later today.

— Reply to this email directly or view it on GitHub https://github.com/riptano/ComboAMI/pull/74#issuecomment-92809671.

elad commented 9 years ago

Okay, this seems to work well. I tested by deploying the AMI, wiping the configuration and resetting the repository to my branch, then running ds0_updater.py. I'm sure you guys have a better testing environment - please give it a spin. :)

joaquincasares commented 9 years ago

Hopping in here for just a bit. :)

Unfortunately that would be the best way to test this pull request with an "official test" happening at bake time.

@mlococo If launching a normal AMI with the --dev parameter once this has been merged into dev-2.5 works without complications, then I'd typically merge the pull request into 2.5. That way we ensure we don't break the current AMI and can always retest functionality after the next bake.

The reason for this is that ds0 and ds0_util should never be reloaded after instantiation to ensure the security check is working as expected.

elad commented 9 years ago

@joaquincasares since you're already here and mentioned rebaking the AMI, I should note that there's at least one more change I'd like to propose in a separate pull request: Adding --allowed-keys and letting folks other sign repositories. I think this is obvious given --repository and there's an existing issue for it (#69).

joaquincasares commented 9 years ago

+1, I'd be open for that pull request. I'll leave that for @mlococo to decide if we can get that implemented before the next rebake.

mlococo commented 9 years ago

Reviewed code, looks awesome. Merged to dev-2.5, I won't have time to test it today as I'm slammed with other work, but will do so this week.

elad commented 9 years ago

Fantastic, thanks! :)