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

[WIP] vmpublish.rb: publish public images #154

Closed
wants to merge 1 commit into from
Closed

[WIP] vmpublish.rb: publish public images #154

wants to merge 1 commit into from

Conversation

geertj
Copy link
Contributor

@geertj geertj commented Sep 8, 2016

This script publishes builds from a previous vmbuild.rb invocation to
any of the configurated public directories. Currently this means Atlas
(for Vagrant images) and GCE (for GCE images).

This code could probably use a similar metadata driven approach as
vmbuild.rb. However for now I believe the current approach of hardcoding
some of the calling conventions and parameters for each of the supported
targets works.

This script publishes builds from a previous vmbuild.rb invocation to
any of the configurated public directories. Currently this means Atlas
(for Vagrant images) and GCE (for GCE images).

This code could probably use a similar metadata driven approach as
vmbuild.rb. However for now I believe the current approach of hardcoding
some of the calling conventions and parameters for each of the supported
targets works.
@miq-bot
Copy link
Member

miq-bot commented Sep 8, 2016

Checked commit https://github.com/geertj/manageiq-appliance-build/commit/19ff7a8fcbf18f92f754e201bd2abdba7ee98734 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 18 offenses detected

scripts/vmbuild.rb

scripts/vmpublish.rb

FILE_SERVER = ENV["BUILD_FILE_SERVER"] # SSH Server to host files
FILE_SERVER_ACCOUNT = ENV["BUILD_FILE_SERVER_ACCOUNT"] # Account to SSH as
FILE_SERVER_BASE = Pathname.new(ENV["BUILD_FILE_SERVER_BASE"] || ".") # Subdirectory of Account where to store builds

Copy link
Member

Choose a reason for hiding this comment

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

@geertj from a reviewer perspective, I have to scroll up and down to see if this code was just moved or moved and modified. It looks identical, but moved. Can you do the move to a new file in a single commit so I can review commit by commit and not have to keep all of these changes in my head?


FILE_SERVER = ENV["BUILD_FILE_SERVER"] # SSH Server to host files
FILE_SERVER_ACCOUNT = ENV["BUILD_FILE_SERVER_ACCOUNT"] # Account to SSH as
FILE_SERVER_BASE = Pathname.new(ENV["BUILD_FILE_SERVER_BASE"] || ".") # Subdirectory of Account where to store builds
Copy link
Member

Choose a reason for hiding this comment

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

So, we need to put these constants within the Build namespace. Meaning, they should reside in module Build. We can do this later but we really should have them in that namespace and all code that uses it, if it's in that namespace, can access it without needing to specify the namespace, meaning we can use BUILD_BASE within the namespace instead of Build::BUILD_BASE. When we put it in the Build namespace, it needs to live in a build directory, so scripts/build/config.rb...

Because it's relevant here but not required... I created #158 for the following comment:

Future PR: Note, we really need to move files to the right places and re-namespace things...

vmbuild.rb             -> build.rb
cli.rb                 -> build/cli.rb
git_checkout.rb        -> build/git_checkout.rb
kickstart_generator.rb -> build/kickstart_generator.rb
productization.rb      -> build/productization.rb
target.rb              -> build/target.rb
productization.rb      -> build/productization.rb
uploader.rb            -> build/uploader.rb

Also, rename scripts to lib.

@@ -0,0 +1,80 @@
require 'logger'
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this publish.rb? We really need to rename vmbuild.rb to build.rb since that file adds the Build module.

@geertj
Copy link
Contributor Author

geertj commented Sep 28, 2016

Sorry for the delay guys. Let's try to get resolution on the design issue first: whether this in the first instance needs to be a separate executable program, or be part of vmbuild.rb.

I proposed three reasons in favor of a separate program, and some of those were rightfully challenged. But taking a step back, I think the reason behind all of these is my intuition shaped by the Unix philosophy: small utilities that do one thing and do it well, and that communicate via pipes (or files).

The benefits are that the individual program become easier to develop and debug. Errors are localized, and everything becomes easier to test (I realize we have unit tests and stuff.. but still) And we can also use the best tool for the job. Take for example #161 where I add support for uploading to EC2. I decided to use Ansible for that given that I needed a mix of API and secure shell actions. Ansible makes that very easy to do. The smaller program can then be tied together with a script if we need to.

One consequence is that we need to be less aggressive cleaning up the files in /build/storage. It contains essential information that is needed for the upload. See the EC2 playbook where I parse the .meta file to get the uncompressed file size for the image. I think if you keep files for at least 2 days you are mostly good. That gives you 2 days to do manual uploads and test things out. (And if an image gets delete that's not a problem as it can be recreated - it just takes time.)

@geertj geertj changed the title vmpublish.rb: publish public images [WIP] vmpublish.rb: publish public images Sep 30, 2016
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Jun 4, 2018

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants