projectatomic / vagrant-service-manager

To provide the user a CLI to configure the ADB/CDK for different use cases and to provide glue between ADB/CDK and the user's developer environment.
GNU General Public License v2.0
18 stars 16 forks source link

For CDK vagrant-service-manager should print OpenShift setup information during `vagrant up` #163

Open LalatenduMohanty opened 8 years ago

LalatenduMohanty commented 8 years ago

Vagrant-service-manager (1.0.0) should print OpenShift setup information during vagrant up for CDK.

For CDK, vagrant-service-manager should print the below OpenShift information as it is the default in CDK.

==> default: 
==> default: Successfully started and provisioned VM with 2 cores and 3072 MB of memory.
==> default: To modify the number of cores and/or available memory set the environment variables
==> default: VM_CPU respectively VM_MEMORY.
==> default: 
==> default: You can now access the OpenShift console on: https://10.1.2.2:8443/console
==> default: 
==> default: To use OpenShift CLI, run:
==> default: $ vagrant ssh
==> default: $ oc login 10.1.2.2:8443
==> default: 
==> default: Configured users are (<username>/<password>):
==> default: openshift-dev/devel
==> default: admin/admin
==> default: 
==> default: If you have the oc client library on your host, you can also login from your host.
LalatenduMohanty commented 8 years ago

Looks like lib/vagrant-service-manager/services/open_shift.rb needs to print the stdout

module Vagrant
  module ServiceManager
    class OpenShift
      def initialize(machine, ui)
        @machine = machine
        @ui = ui
        @extra_cmd = build_extra_command
      end

      def execute
        full_cmd = "#{@extra_cmd} sccli openshift"

        @machine.communicate.sudo(full_cmd) do |type, data|
          if type == :stderr
            @ui.error(data)
            exit 126
          end
        end
      end

      private

      def build_extra_command
        cmd = ''
        CONFIG_KEYS.select {|e| e[/^openshift_/] }.each do |key|
          unless @machine.config.servicemanager.send(key).nil?
            env_name = key.to_s.gsub(/openshift_/,'').upcase
            cmd += "#{env_name}='#{@machine.config.servicemanager.send(key)}' "
          end
        end
        cmd.chop
      end
    end
  end
end
hferentschik commented 8 years ago

Now, whether the service-manager does print this setup information or not is debatable, but how is this going to solve the issue? There is nothing wrong with the current Vagrantfile afaict. It should be possible to add the script provisioning at this step. We need to find the underlying reason why Vagrant exits prematurely.

LalatenduMohanty commented 8 years ago

@hferentschik Agree.

It does not solve https://github.com/projectatomic/adb-atomic-developer-bundle/issues/337.

But IMO it should print required openshift info. It is a step towards having a minimal Vagrantfile.

Ideall sccli openshift should print the info in stdout and vsm should pick them and display to user.

navidshaikh commented 8 years ago

Ideall sccli openshift should print the info in stdout and vsm should pick them and display to user.

+1 , this should also help handling services by one single component than having multiple components (sccli, vsm, Vagrantfile) configuring a single service. This helps configuring a service completely by one component, ensuring the rest part of configuration is not left over by another components and also not having the dependency on other components to do the rest of configuration.

coolbrg commented 8 years ago

@LalatenduMohanty @navidshaikh : Moreover, we found that vagrant service-manager plugin is not executing extra provisioners present in Vagrantfile (https://github.com/projectatomic/vagrant-service-manager/issues/166).

navidshaikh commented 8 years ago

Moreover, we found that vagrant service-manager plugin is not executing extra provisioners present in Vagrantfile (#166)

Because proper exit codes were not returned from the opensihft configuration commands from sccli

hferentschik commented 8 years ago

It does not solve projectatomic/adb-atomic-developer-bundle#337.

Ok, as long as we agree on that, I am fine :-)

But IMO it should print required openshift info.

Sounds reasonable. We just have to make sure that 'vagrant up' completes successfully and that potential additional provisioning steps get executed as expected.

It is a step towards having a minimal Vagrantfile.

+1

LalatenduMohanty commented 8 years ago

Corresponding issue in adb-utils https://github.com/projectatomic/adb-utils/issues/55

navidshaikh commented 8 years ago

and for removing the lines from Vagrantfile https://github.com/projectatomic/adb-atomic-developer-bundle/issues/341

hferentschik commented 8 years ago

BTW, the Vagrantfile is not such a bad location for this final access information, especially if we can get it into the box'es Vagrantfile. Just wondering whether it is really the responsibility of sccli or the plugin really. Also there is already vagrant service-manager env openshift.

LalatenduMohanty commented 8 years ago

@hferentschik We should not put the OpenShift access information in to the Vagrantbox's Vagrantfile. Because that would mean always getting OpenShift information even if we just want docker , k8s or mesos. Though we can put complex ruby code in the Vagrantfile too to handle. But IMO code outside of the Vagrantfile (i.e. VSM) is better.

I think VSM/SCCLI should have this info as these are interfaces to CDK/ADB.

hferentschik commented 8 years ago

We should not put the OpenShift access information in to the Vagrantbox's Vagrantfile.

true, true

So why not call service-manager env openshift and displaying its content instead of adding more logging elsewhere?

LalatenduMohanty commented 8 years ago

So why not call service-manager env openshift and displaying its content instead of adding more logging elsewhere?

I am not opposed to the idea. The end result is vagrant up should have the required information.

hferentschik commented 8 years ago

I am not opposed to the idea

:-)

hferentschik commented 8 years ago

There is a drawback though, unless we have an explicit command which we can call at the very end, we might not get the desired output at the end of the provisioning. In this case the information might get lost among other messages.

coolbrg commented 8 years ago

@LalatenduMohanty @navidshaikh @hferentschik Since, now the idea is to auto-generate Vagrantfiles, this can be implemented along with https://github.com/projectatomic/vagrant-service-manager/issues/163

hferentschik commented 8 years ago

Since, now the idea is to auto-generate Vagrantfiles, this can be implemented along with #163

not quite sure I see the connection here.

coolbrg commented 8 years ago

@hferentschik , if it is only moving print message part then it is different than #163.

Then, it means that the shell provisioning part will be migrated to vagrant-service-manager, right?

hferentschik commented 8 years ago

Then, it means that the shell provisioning part will be migrated to vagrant-service-manager, right?

I guess that was the intend.

navidshaikh commented 8 years ago

Even if we add feature for auto-generating Vagrantfile, having the openshift information message print part should be moved to vagrant-service-manager from Vagrantfile. As Hardy said, there is no connection.

coolbrg commented 8 years ago

@navidshaikh @hferentschik : If we go this way of printing the message, then we need to remove the shell provisioning lines from provided Vagrantfiles since vagrant up process will print duplicate messages.

What is your opinions?

navidshaikh commented 8 years ago

If we go this way of printing the message, then we need to remove the shell provisioning lines from provided Vagrantfiles since vagrant up process will print duplicate messages.

We remove the one which is extra! We remove the shell provisioning part from Vagrantfile.